GDB-14779: Solr deprecation banner is not sticky#3012
Conversation
1c684f0 to
e29dc46
Compare
| * @returns {@code true} if preferences should be persisted. | ||
| */ | ||
| private shouldPersistPreferences(): boolean { | ||
| return this.securityContextService.getSecurityConfig()?.enabled === true |
There was a problem hiding this comment.
you can use config.isEnabled()
| * @returns {@code true} if preferences should be persisted. | ||
| */ | ||
| private shouldPersistPreferences(): boolean { | ||
| return this.securityContextService.getSecurityConfig()?.enabled === true |
There was a problem hiding this comment.
Without security enabled the user is an admin (or a repo manager if there is override auth). Shouldn't those be able to persist their preferences? If a user should be explicitly logged in, you can replace all of this with authentication.service#isLoggedIn
There was a problem hiding this comment.
I will check this. If security is disabled and getIsLoggedIn() returns false, I'll change it.
There was a problem hiding this comment.
I think we can go with logged in users only.
| /** | ||
| * Context service responsible for storing user-specific data for the current application session. | ||
| */ | ||
| export class UserContextService extends ContextService<UserContextFields> implements DeriveContextServiceContract<UserContextFields, UserContextFieldParams>, LifecycleHooks { |
There was a problem hiding this comment.
This must be UserPreferencesContextService
There was a problem hiding this comment.
Initially, the name was UserPreferencesContextService, but I thought it could be used in the future for other user-related properties.
There was a problem hiding this comment.
Renamed and moved
There was a problem hiding this comment.
Why are these services in the domain package? They have nothing to do with the domains. Move them outside
There was a problem hiding this comment.
I thought of them as user-related, so I placed them next to the user service. I'll rename UserPreferencesContextService and move them back to where they were.
|
|
||
| return this.userPreferencesStorageService | ||
| .getUsersPreferences() | ||
| .getUserPreferences(username) ?? new UserPreferences(); |
There was a problem hiding this comment.
This should be something like this
this.userPreferencesStorageService
.getPreferences()
.getPreferenceByUsername(username)
e29dc46 to
131c05e
Compare
| let securityContextService: jest.Mocked<SecurityContextService>; | ||
| let authenticationService: jest.Mocked<AuthenticationService>; | ||
|
|
||
| let storage: Record<string, string>; |
There was a problem hiding this comment.
We have a StorageMock. Why don't you use it?
There was a problem hiding this comment.
Reimplemented
| let authenticationService: jest.Mocked<AuthenticationService>; | ||
|
|
||
| let storage: Record<string, string>; | ||
| let context: Record<string, unknown>; |
There was a problem hiding this comment.
Why do you need to mock the context?! Why not just using the actual context service and just spy on it if you need.
There was a problem hiding this comment.
Reimplemented
|
|
||
| return this.userPreferencesStorageService | ||
| .getUsersPreferences() | ||
| .getUserPreferences(username); |
There was a problem hiding this comment.
this is still unchanged
| * Indicates whether the user has dismissed the Solr deprecation banner. | ||
| * When {@code true}, the banner is not shown again for the user. | ||
| */ | ||
| dismissSolrDeprecationBanner = false; |
There was a problem hiding this comment.
rename this to either dismissedSolrDeprecationBanner or isSolrDeprecationBannerDismissed
## What - Kept the deprecation banner sticky together with the application header. - Added user preferences infrastructure for storing user-specific UI settings. - Implemented persistence of the Solr deprecation banner dismissed state for authenticated users. ## Why - The deprecation banner was rendered outside of the sticky header container, causing it to scroll out of view instead of remaining visible with the header. - Users should not have to dismiss the Solr deprecation banner every time they visit the application when they are authenticated. - Anonymous users cannot be identified, so their dismissed state should be preserved only for the current browser session. ## How - Moved the deprecation banner into the sticky header container so both the header and the banner remain fixed at the top while scrolling. - Introduced models, context, storage, and service classes for managing user-specific preferences. - Persisted the dismissed banner state per authenticated user while keeping the preference in memory for anonymous users.
131c05e to
8ca3896
Compare
|



What
Why
How
Checklist