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

Display upgrade verification prompt #7454

Merged
merged 2 commits into from
Mar 28, 2023
Merged

Conversation

Anderas
Copy link
Contributor

@Anderas Anderas commented Mar 27, 2023

Related SDK change matrix-org/matrix-ios-sdk#1751

Resolves https://github.com/vector-im/crypto-internal/issues/58

Display customized modal alert if upgrading from legacy to rust crypto and needing to verify the account.

@Anderas Anderas requested review from a team, pixlwave and BillCarsonFr and removed request for a team March 27, 2023 14:35
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.

LGTM

Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

I'm not sure I fully understand how this works - I don't see where showVerificationUpgradeAlert gets set back to true and when it would get set back to false after an alert. But the code looks fine to me.

Comment on lines 991 to 994
if
let feature = MXSDKOptions.sharedInstance().cryptoSDKFeature,
feature.isEnabled && feature.needsVerificationUpgrade
{
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be more inline with the project's style:

Suggested change
if
let feature = MXSDKOptions.sharedInstance().cryptoSDKFeature,
feature.isEnabled && feature.needsVerificationUpgrade
{
if let feature = MXSDKOptions.sharedInstance().cryptoSDKFeature,
feature.isEnabled && feature.needsVerificationUpgrade {

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

❗ No coverage uploaded for pull request base (hotfix/1.10.7@022d7e7). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head bff0b28 differs from pull request most recent head 8202617. Consider uploading reports for the commit 8202617 to get more accurate results

Additional details and impacted files
@@               Coverage Diff                @@
##             hotfix/1.10.7    #7454   +/-   ##
================================================
  Coverage                 ?   12.30%           
================================================
  Files                    ?     1645           
  Lines                    ?   162976           
  Branches                 ?    66914           
================================================
  Hits                     ?    20047           
  Misses                   ?   142280           
  Partials                 ?      649           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Anderas
Copy link
Contributor Author

Anderas commented Mar 28, 2023

I'm not sure I fully understand how this works - I don't see where showVerificationUpgradeAlert gets set back to true and when it would get set back to false after an alert. But the code looks fine to me.

Discussed the mechanism offline

@Anderas Anderas merged commit 183123b into hotfix/1.10.7 Mar 28, 2023
@Anderas Anderas deleted the andy/upgrade_security branch March 28, 2023 14:00
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.

3 participants