Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for bug stopping vault timeout to never #1618

Merged
merged 1 commit into from Oct 29, 2021
Merged

Conversation

jlf0dev
Copy link
Member

@jlf0dev jlf0dev commented Oct 29, 2021

Type of change

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

Objective

When switching to the settings screen and selecting a timeout option of "Never", if you left the screen and returned the value reverted back to "Immediately".

The timeout preference option of "Never" is being saved as null in storage, but when we made the call to storage for the value we defaulted it back to "Immediately" if the value was null.

This fix removes the default value and instead allows the returned value to be null if the user selected "Never" as a timeout option.

Closes #1615
Asana Task: https://app.asana.com/0/1169444489336079/1201287480797899/f

Code changes

  • src/App/Pages/Settings/SettingsPage/SettingsPageViewModel.cs: Allow _vaultTimeout to be nullable, remove the GetValueorDefault() while selecting value.
  • src/Core/Abstractions/IVaultTimeoutService.cs: Allow returned int to be nullable
  • src/Core/Services/VaultTimeoutService.cs: Allow returned int to be nullable, add additional null checks, and refactor timeout policy to work with null value

Screenshots

Testing requirements

  • Setting VaultTimeout to "Never" shouldn't revert back to "Immediately" after changing screens
  • Correct VaultTimeout should persist between uses

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)

- use nullable int on settings page and in vault service
@jlf0dev jlf0dev requested review from Hinton and mpbw2 October 29, 2021 03:31
@@ -17,6 +17,6 @@ public interface IVaultTimeoutService
Task LockAsync(bool allowSoftLock = false, bool userInitiated = false);
Task LogOutAsync();
Task SetVaultTimeoutOptionsAsync(int? timeout, string action);
Task<int> GetVaultTimeout();
Task<int?> GetVaultTimeout();
Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes me nervous, especially for a hotfix. As this is a new bug introduced with the custom timeout feature, it seems like a UI issue in Settings. Is there a way to handle the new option without modifying the service?

Copy link
Member Author

@jlf0dev jlf0dev Oct 29, 2021

Choose a reason for hiding this comment

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

We could replace the null value we're saving for "Never" with a negative number, but that will require a much larger change on the settings page. Even if we did the bare minimum and left the variables as nullable, the max timeout policy was written under the assumption that the value for "Never" was null. We just missed that with the underlying service.

I believe I've checked all the calls to the service, but we could try the negative number. What do you think?

Copy link
Member

@Hinton Hinton Oct 29, 2021

Choose a reason for hiding this comment

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

@mportune-bw I think I agree with @jlf0dev, the value should have been null-able from the beginning. (On the other clients, never is also represented as null, and negative values tends to mark special actions like lock on restart, lock on inactivity.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you're saying now. Let's get this in but let QA know regression is needed for vault timeout before we ship.

@jlf0dev jlf0dev merged commit 318a3e4 into master Oct 29, 2021
@jlf0dev jlf0dev deleted the bug/vault-timeout branch October 29, 2021 14:31
mpbw2 pushed a commit that referenced this pull request Oct 29, 2021
- use nullable int on settings page and in vault service
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.

Android - Setting the Vault Timeout to Never is not saved, and is automatically changed to Immidiately.
3 participants