Conversation
| Task<BwRegion?> GetActiveUserRegionAsync(); | ||
| Task<BwRegion?> GetPreAuthRegionAsync(); | ||
| Task SetPreAuthRegionAsync(BwRegion value); | ||
| Task<bool> GetShouldCheckOrganizationUnassignedItemsAsync(string userId = null); |
There was a problem hiding this comment.
🤔 Naming wise, following a similar pattern to the other prompts (e.g. Get/Set UnassignedItemsPromptShownAsync) would make it easier to find this in the future.
There was a problem hiding this comment.
The problem is that this getter doesn't return if the prompt has been shown or not, it returns whether the check (and potentially the prompt) should be done or not, i.e. if the user chose "Remind me later" or not.
Also there are also two other that have Should:
- GetShouldTrustDeviceAsync
- GetShouldConnectToWatchAsync
Even though I agree having everything with the same name structure is better, but on the other hand I like having specific naming so one can exactly know what the method/property does without having to look much in other places.
What do you think?
| public static string ShouldConnectToWatchKey(string userId) => $"shouldConnectToWatch_{userId}"; | ||
| public static string ScreenCaptureAllowedKey(string userId) => $"screenCaptureAllowed_{userId}"; | ||
| public static string PendingAdminAuthRequest(string userId) => $"pendingAdminAuthRequest_{userId}"; | ||
| public static string ShouldCheckOrganizationUnassignedItemsKey(string userId) => $"shouldCheckOrganizationUnassignedItems_{userId}"; |
There was a problem hiding this comment.
⛏️ (not really relevant but worth mentioning) other similar constants don't have verbiage like "ShouldCheck".
There was a problem hiding this comment.
IMO without the "should check" one doesn't know what the flag is for, only that it has to do with organization unassigned items. However, if having the verbiage alike others is more important I can change it.
|
|
||
| public async Task<bool> VerifyOrganizationHasUnassignedItemsAsync() | ||
| { | ||
| var organizations = await _stateService.GetOrganizationsAsync(); |
There was a problem hiding this comment.
🤔 I'm not sure if it's possible for this to return 0 orgs and the next call returning true, I just noticed other clients aren't checking this.
There was a problem hiding this comment.
This is exactly why I had to wait for synchronization to complete, otherwise we'd have 0 orgs right after logging in given the sync wouldn't have finished.
I could remove this alongside the sync wait but it will fire a request to the server for users that don't have organizations which IMO is a waste of resources and given that we're waiting for sync to finish we should have the latest organizations state when checking it.
| } | ||
| var previousPage = await AppHelpers.ClearPreviousPage(); | ||
|
|
||
| _appOptions.HasJustLoggedInOrUnlocked = true; |
There was a problem hiding this comment.
💭 Instead of having this variable scattered around and risk missing a path to the vault, can't we just check once on TabsPage.OnCreate with a control variable HasRunCheckOrganizationUnassignedItems?
There was a problem hiding this comment.
The problem is that TabsPage.OnCreate can be called when the vault has timeout never, then it's not technically after login/unlock as it will be called when opening the app and given timeout never the vault would be already unlocked.
vvolkgang
left a comment
vvolkgang
left a comment
There was a problem hiding this comment.
Thanks for adding the self-host message!
fedemkr
deleted the
mobiletf/pm-7407/popup-organization-unassigned-items
branch
3 participants
Type of change
Objective
Add check for organization with unassigned items to display a message to the user if it's needed.
Code changes
GroupingsPageAppOptionsand passed along the options object to get to theGroupingsPageScreenshots
Unassigned items notice iOS
Unassigned items notice Android
Before you submit
dotnet format --verify-no-changes) (required)