Skip to content

[SG-79] Add filter to search and preselect org in new cipher#1944

Merged
mpbw2 merged 3 commits intomasterfrom
bugfix-vaultfilter
Jun 8, 2022
Merged

[SG-79] Add filter to search and preselect org in new cipher#1944
mpbw2 merged 3 commits intomasterfrom
bugfix-vaultfilter

Conversation

@mpbw2
Copy link
Contributor

@mpbw2 mpbw2 commented Jun 7, 2022

Type of change

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

Objective

  • SG-362 Added vault org filter support to cipher search page
  • SG-356 Pre-select org when creating new cipher from org-filtered list

Code changes

  • GroupingsPage.xaml: Updated button style
  • GroupingsPage.xaml.cs: Pass selected org ID to AddEditPage
  • GroupingsPageViewModel.cs: Make GetVaultFilterOrgId() public
  • AddEditPage.xaml.cs: Add support for organizationId in constructor
  • CiphersPage.xaml/xaml.cs/ViewModel.cs: Added UI & logic for displaying & handling vault filter options & selection
  • Base.xaml: Added new list-row-button-text style to fix color of vault filter button to match mocks

Before you submit

  • I have checked for formatting errors (dotnet tool run dotnet-format --check) (required)
  • 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 and fedemkr June 7, 2022 19:17
Comment on lines +37 to +40
if (vaultFilterSelection != null)
{
_vm.VaultFilterDescription = vaultFilterSelection;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is the if necessary? If _vm.VaultFilterDescription is null and vaultFilterSelection is also null there shouldn't be any problems, or are there any because of the property change or something like that?

Comment on lines +234 to +237
if (!string.IsNullOrWhiteSpace((Page as CiphersPage).SearchBar.Text))
{
Search((Page as CiphersPage).SearchBar.Text, 200);
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we have a ViewModel property SearchText or something like that that we bind TwoWay against the page SearchBar.Text? I'd really like to remove all references from the ViewModel to the pages so that we can at some moment remove the reference to Page from all the VMs and be more aligned with the MVVM pattern.

Comment on lines +247 to +254
var selection = await Page.DisplayActionSheet(AppResources.FilterByVault, AppResources.Cancel, null,
options.ToArray());
if (selection == AppResources.Cancel ||
(_vaultFilterSelection == null && selection == AppResources.AllVaults) ||
(_vaultFilterSelection != null && _vaultFilterSelection == selection))
{
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

If I'm not mistaken selection can be null as what I see here that is triggered on Android when tapping outside the dialog, just in case confirm if here the behavior is different or somehow touching outside is disabled.
Therefore a check should added as well against null

@mpbw2 mpbw2 requested a review from fedemkr June 8, 2022 13:38
@mpbw2 mpbw2 merged commit 88b4065 into master Jun 8, 2022
@mpbw2 mpbw2 deleted the bugfix-vaultfilter branch June 8, 2022 13:39
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