Disabling License Banners #19116
Disabling License Banners #19116
Conversation
| @@ -9,11 +17,24 @@ | |||
| * @param {string} expiry - RFC3339 date timestamp | |||
| */ | |||
|
|
|||
| import Component from '@glimmer/component'; | |||
There was a problem hiding this comment.
moved the imports above documentation to match the component template
ui/app/components/license-banners.js
Outdated
| // If nothing is saved in localStorage or the user has updated their Vault version, do not dismiss any of the banners. | ||
| const localStorageLicenseBannerObject = localStorage.getItem('licenseBanner'); | ||
| if (!localStorageLicenseBannerObject || localStorageLicenseBannerObject.version !== this.currentVersion) { | ||
| localStorage.setItem('licenseBanner', { dismissType: '', version: this.currentVersion }); |
There was a problem hiding this comment.
I think the logic here could be simplified by storing the dismissal at a variable key, rather than storing an object that has to be parsed and reasoned about. So for example, if I'm on version 1.12.2, this component would check for a value (any value, really) at licenseBanner-warning-1.12.2
The get and set calls would be able to use a key variable that is calculated based on the version:
const warningBannerKey = `licenseBanner-warning-${this.currentVersion}`
const dismissedWarning = localStorage.getItem(warningBannerKey)
There was a problem hiding this comment.
I like the idea. Let me know what you think of the changes. Not exactly your suggestion, but a similar concept.
|
|
||
| @tracked warningDismissed; | ||
| @tracked expiredDismissed; | ||
|
|
There was a problem hiding this comment.
One thought about this approach, is that localStorage will continue to store past version dismissals. Thinking through this, there really isn't a way for me to get the previous version so I'd have to do some string finding to remove a past version's (dismiss-license-banner-${this.currentVersion}). Thoughts?
There was a problem hiding this comment.
That's true. We could have a helper in the localStorage lib which goes through each of the localStorage values and matches on the first part of the string, then removes the matching ones
const relevantKeys = Object.keys(localStorage).filter(str => str.startsWith("dismiss-license-banner")
relevantKeys.forEach(key => {
localStorage.removeItem(key)
}
There was a problem hiding this comment.
Great idea. I'll add that.
There was a problem hiding this comment.
Added a method to lib/local-storage. Let me know what you think. (see row 51 here and the new method on local-storage.js).
| @action | ||
| dismissBanner(dismissAction) { | ||
| // updates localStorage and then updates the template by calling updateDismissType | ||
| localStorage.setItem(`dismiss-license-banner-${this.currentVersion}`, dismissAction); |
There was a problem hiding this comment.
If I'm understanding this correctly, if I've previously dismissed a warning and I come back and dismiss an expired, we will replace the localStorage value so that the warning will be able to show up again?
There was a problem hiding this comment.
Correct. The key here is that both will never show up at the same time. So let's say you dismissed a warning, but then you let your license expire, the expired license will show. If we dismiss that, then next time a warning should display it will.
There was a problem hiding this comment.
Great work! I have one more request 🙏
Can we add a few test scenarios covering the new local-storage method, including it behaves correctly when:
- nothing in localstorage
- extra things in localstorage (those should not be removed)
- keeping a specified key that would otherwise be removed
Once that's done let's ship it 🚀
Monkeychip
deleted the
ui/VAULT-13299/allow-disabling-license-banners
branch
* work in progress: got the expired banner set with license check * wip: got the logic for both banners, need to test and write tests * add notes * prep for test writing * test coverage * add changelog * clean up * clarify dismissTypes and conditionals * updates * update comment * update comment * address pr comments * update test * small naming change * small naming changes * clean localStorage * comment clean up * another comment clean up * remove meep * add test coverage for new method in localStorage
* work in progress: got the expired banner set with license check * wip: got the logic for both banners, need to test and write tests * add notes * prep for test writing * test coverage * add changelog * clean up * clarify dismissTypes and conditionals * updates * update comment * update comment * address pr comments * update test * small naming change * small naming changes * clean localStorage * comment clean up * another comment clean up * remove meep * add test coverage for new method in localStorage
This PR allows the
licenseBannersto be dismissed and records those preferences in localStorage. Once a banner is dismissed, it will not show again unless a user clears their localStorage, or the vault version changes.license.mov