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

Enable reset all and skip options #7721

Merged
merged 4 commits into from Dec 7, 2022

Conversation

amitkma
Copy link
Contributor

@amitkma amitkma commented Dec 6, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Implemented ResetAll for self verification.
  • Skip option is enabled for self verification.
  • Fixed back navigation with SharedSecureStorage.

Motivation and context

Part of element-r refactor alignment with web.

Tests

  • Login to an account in Element-android
  • Start self verification, Click on "Forgot or lost all recovery options".
  • It should work as before.

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 12

Checklist

@ElementBot
Copy link

Warnings
⚠️

Please add a changelog. See instructions here

⚠️ You seem to have made changes to views. Please consider adding screenshots.
⚠️ Please add a reviewer to your PR.

Generated by 🚫 dangerJS against 2f30718

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

Just a few comments.
Also noticed that if you go to Use Passphrase > Reset > Cancel it goes back to the bottomsheet. Maybe it should go back to enter passphrase?

@@ -410,7 +410,7 @@
<string name="action_disconnect">Disconnect</string>
<string name="action_play">Play</string>
<string name="action_dismiss">Dismiss</string>
<string name="action_reset">Reset</string>
<string name="action_reset">Proceed to Reset</string>
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather create a new string, this one might be used somewhere else?

Copy link
Contributor Author

@amitkma amitkma Dec 7, 2022

Choose a reason for hiding this comment

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

I already checked that it is not used anywhere other than reset fragment. I will add a new string as per contribution guidelines.

@@ -2634,9 +2634,10 @@
<string name="failed_to_access_secure_storage">Failed to access secure storage</string>
<string name="bad_passphrase_key_reset_all_action">Forgot or lost all recovery options? Reset everything</string>
<string name="secure_backup_reset_all">Reset everything</string>
<string name="secure_backup_reset_all_no_other_devices">Only do this if you have no other device you can verify this device with.</string>
<string name="secure_backup_reset_all_no_other_devices">Resetting your verification keys cannot be undone. After resetting, you won't have access to old encrypted messages, and any friends who have previously verified you will see security warnings until you re-verify with them.</string>
Copy link
Member

Choose a reason for hiding this comment

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

create a new string here? and mark this one as eventually? Check the Contributing.md for strings.

I think we can keep a short version and a longer one. Use the short in the bottomsheet and the long in the reset fragment?

Copy link
Member

Choose a reason for hiding this comment

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

Also won'tneeds an escape

@@ -125,15 +126,17 @@ class SharedSecureStorageViewModel @AssistedInject constructor(
copy(
hasPassphrase = true,
ready = true,
step = SharedSecureStorageViewState.Step.EnterPassphrase
step = if (initialState.step == SharedSecureStorageViewState.Step.ResetAll) SharedSecureStorageViewState.Step.ResetAll
else SharedSecureStorageViewState.Step.EnterPassphrase
Copy link
Member

Choose a reason for hiding this comment

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

I find this a bit strange.
Wouldn't it be better to test if reset mode before trying to get the keyResult?
If state is not reset try to get keyResult and set the hasPassphrase/ready?

@@ -82,9 +69,10 @@
android:layout_marginTop="16dp"
android:layout_marginEnd="16dp"
android:layout_marginBottom="16dp"
android:text="@string/secure_backup_reset_no_history"
android:text="@string/secure_backup_reset_danger_warning"
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's for consistency with web?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@ElementBot
Copy link

Warnings
⚠️

Please add a changelog. See instructions here

⚠️ You seem to have made changes to views. Please consider adding screenshots.
⚠️ Please add a reviewer to your PR.

Generated by 🚫 dangerJS against d2966fc

@BillCarsonFr BillCarsonFr self-requested a review December 7, 2022 13:55
Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

Ok to merge in bca/rust_flavor

@amitkma amitkma merged commit 17d25e2 into feature/bca/rust_flavor Dec 7, 2022
@amitkma amitkma deleted the feature/amitkma/rust_flavor branch December 7, 2022 14:10
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.

None yet

3 participants