-
Notifications
You must be signed in to change notification settings - Fork 920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Discover] Display Cache Time and Clear Cache Button #8214
Conversation
Signed-off-by: Sean Li <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8214 +/- ##
==========================================
- Coverage 60.98% 60.98% -0.01%
==========================================
Files 3743 3743
Lines 88857 88862 +5
Branches 13859 13860 +1
==========================================
+ Hits 54188 54190 +2
- Misses 31314 31315 +1
- Partials 3355 3357 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add tests for new methods + snapshot test to bump up patch cov?
src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts
Outdated
Show resolved
Hide resolved
src/plugins/data/public/ui/dataset_selector/dataset_explorer.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts
Outdated
Show resolved
Hide resolved
a good catch all but did we want to consider making it more granular? if i press the refresh on This way to avoid destroying the cache if only one source is out of date? If so then we can consider extending this function https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts#L91 to have a new boolean as a param like To get the date we could actually shove that in the data structure meta for when it was first fetched. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @virajsanghvi comments. nothing blocking
Signed-off-by: Sean Li <[email protected]>
src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts
Outdated
Show resolved
Hide resolved
src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Sean Li <[email protected]>
this.sessionStorage.clear(); | ||
} | ||
|
||
public getLastCacheTime(): number | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit i think there is precedent with updatedAt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreeing with @virajsanghvi with some snapshots will be awesome.
one nit with the naming of the field
one fast follow we should do is pull from ui settings for the date format
src/plugins/data/public/ui/dataset_selector/dataset_explorer.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data/public/query/query_string/dataset_service/dataset_service.ts
Show resolved
Hide resolved
Signed-off-by: Sean Li <[email protected]>
* initial commit for cache time and clearing cache Signed-off-by: Sean Li <[email protected]> * Changeset file for PR #8214 created/updated * updating UI to address some comments Signed-off-by: Sean Li <[email protected]> * addressing comments, adding tests Signed-off-by: Sean Li <[email protected]> * removing dynamic default message, following i18n best practice Signed-off-by: Sean Li <[email protected]> --------- Signed-off-by: Sean Li <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit a7f3e9d) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* initial commit for cache time and clearing cache * Changeset file for PR #8214 created/updated * updating UI to address some comments * addressing comments, adding tests * removing dynamic default message, following i18n best practice --------- (cherry picked from commit a7f3e9d) Signed-off-by: Sean Li <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
…ect#8214) (opensearch-project#8421) * initial commit for cache time and clearing cache * Changeset file for PR opensearch-project#8214 created/updated * updating UI to address some comments * addressing comments, adding tests * removing dynamic default message, following i18n best practice --------- (cherry picked from commit a7f3e9d) Signed-off-by: Sean Li <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
Still early implementation, open to suggestions
Issues Resolved
Screenshot
Empty Cache State:
Cache has been populated:
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration