Skip to content

Global theme and account removal#1793

Merged
mpbw2 merged 4 commits intoaccountswitchingfrom
accountswitching-tweaks
Feb 22, 2022
Merged

Global theme and account removal#1793
mpbw2 merged 4 commits intoaccountswitchingfrom
accountswitching-tweaks

Conversation

@mpbw2
Copy link
Copy Markdown
Contributor

@mpbw2 mpbw2 commented Feb 21, 2022

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

  • Apply theme selections globally (took this approach so we can restore per-account themes in the future by removing a contained block of code)
  • Support for account removal from login page (via logout on vault timeout)
  • Check for existing account on add-account and prompt to switch if available

Code changes

  • LoginPage.*: Added "Remove Account" menu option and supporting code, also added checking for already-added account when logging in a new account
  • OptionsPageViewModel.cs: Apply theme globally when applied in Settings
  • StateService.cs: Added GetUserIdAsync and ApplyThemeGloballyAsync methods

Screenshots

Screen Shot 2022-02-20 at 8 45 54 PM

Screen Shot 2022-02-20 at 8 46 17 PM

Screen Shot 2022-02-20 at 8 48 03 PM

Testing requirements

  • Change theme then switch users - theme will remain consistent
  • Switch to an active logged-out account and expand the overflow menu to use the "Remove Account" option
  • Add an account you've already added

Before you submit

  • I have added unit tests where it makes sense to do so (encouraged but not required)
  • This change requires a documentation update (notify the documentation team)
  • This change has particular deployment requirements (notify the DevOps team)

@mpbw2 mpbw2 requested review from a team, fedemkr and vvolkgang February 21, 2022 02:17
Comment on lines +211 to +218
public async Task RemoveAccountAsync()
{
var confirmed = await _platformUtilsService.ShowDialogAsync(AppResources.RemoveAccountConfirmation,
AppResources.RemoveAccount, AppResources.Yes, AppResources.Cancel);
if (confirmed)
{
_messagingService.Send("logout");
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add try...catch

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify? This is triggered directly by the user with the app in the foreground.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mportune-bw if the ShowDialogAsync or the _messagingService throw an exception the app will crash given that the RemoveAccountAsync is called from the event RemoveAccount_Clicked. So a try...catch is needed here or in RemoveAccount_Clicked to prevent the crash if any exception happen.

Comment thread src/Core/Services/StateService.cs Outdated
{
if (email == null)
{
throw new Exception("email cannot be null");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use the explicit exception here which is ArgumentNullException

Comment thread src/Core/Services/StateService.cs Outdated
foreach (var account in _state.Accounts)
{
var accountEmail = account.Value?.Profile?.Email;
if (accountEmail != null && accountEmail == email)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that email can't be null because of aforementioned check, then this can be directly written as
if (accountEmail == email)

Comment thread src/Core/Services/StateService.cs Outdated
Comment on lines +872 to +885
if (_state?.Accounts != null)
{
var activeUserId = await GetActiveUserIdAsync();
foreach (var account in _state.Accounts)
{
var uid = account.Value?.Profile?.UserId;
if (uid == null || uid == activeUserId)
{
// active user theme already set, skip
continue;
}
await SetThemeAsync(value, uid);
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to avoid nesting and some lines, I'd rewrite this as:

if (_state?.Accounts == null)
{
    return;
}

var activeUserId = await GetActiveUserIdAsync();
foreach (var account in _state.Accounts)
{
    var uid = account.Value?.Profile?.UserId;
    if (uid != null && uid != activeUserId)
    {
        // active user theme not already set, so set it
        await SetThemeAsync(value, uid);
    }
}

@mpbw2 mpbw2 requested a review from fedemkr February 22, 2022 02:48
@mpbw2 mpbw2 merged commit 2ded824 into accountswitching Feb 22, 2022
@mpbw2 mpbw2 deleted the accountswitching-tweaks branch February 22, 2022 15:05
mpbw2 added a commit that referenced this pull request Feb 23, 2022
* Account Switching (#1720)

* Account switching

* WIP

* wip

* wip

* updates to send test logic

* fixed Send tests

* fixes for theme handling on account switching and re-adding existing account

* switch fixes

* fixes

* fixes

* cleanup

* vault timeout fixes

* account list status enhancements

* logout fixes and token handling improvements

* merge latest (#1727)

* remove duplicate dependency

* fix for initial login token storage paradox (#1730)

* Fix avatar color update toolbar item issue on iOS for account switching (#1735)

* Updated account switching menu UI (#1733)

* updated account switching menu UI

* additional changes

* add key suffix to constant

* GetFirstLetters method tweaks

* Fix crash on account switching when logging out when having more than user at a time (#1740)

* single account migration to multi-account on app update (#1741)

* Account Switching Tap to dismiss (#1743)

* Added tap to dismiss on the Account switching overlay and improved a bit the code

* Fix account switching overlay background transparent on the proper place

* Fixed transparent background and the shadow on the account switching overlay

* Fix iOS top space on Account switching list overlay after modal (#1746)

* Fix top space added to Account switching list overlay after closing modal

* Fix top space added to Account switching list overlay after closing modal on lock, login and home views just in case we add modals in the future there as well

* Usability: dismiss account list on certain events (#1748)

* dismiss account list on certain events

* use new FireAndForget method for back button logic

* Create and use Account Switching overlay control (#1753)

* Added Account switching overlay control and its own ViewModel and refactored accordingly

* Fix account switching Accounts list binding update

* Implemented dismiss account switching overlay when changing tabs and when selecting the same tab. Also updated the deprecated listener on CustomTabbedRenderer on Android (#1755)

* Overriden Equals on AvatarImageSource so it doesn't get set multiple times when it's the same image thus producing blinking on tab chaged (#1756)

* Usability improvements for logout on vault timeout (#1781)

* accountswitching fixes (#1784)

* Fix for invalid PIN lock state when switching accounts (#1792)

* fix for pin lock flow

* named tuple values and updated async

* clear send service cache on account switch (#1796)

* Global theme and account removal (#1793)

* Global theme and account removal

* remove redundant call to hide account list overlay

* cleanup and additional tweaks

* add try/catch to remove account dialog flow

Co-authored-by: Federico Maccaroni <fedemkr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants