-
Notifications
You must be signed in to change notification settings - Fork 738
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
Feature/bca/verif resist no age #6328
Conversation
111869d
to
6cc1fce
Compare
d858ed7
to
9929d6a
Compare
related to matrix-org/synapse#8429 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some remarks, since this is "NextRelease", I have made a review. Did not test the code though.
...android/src/main/java/org/matrix/android/sdk/internal/database/helper/ThreadSummaryHelper.kt
Outdated
Show resolved
Hide resolved
...n/java/im/vector/app/features/crypto/verification/choose/VerificationChooseMethodFragment.kt
Outdated
Show resolved
Hide resolved
...n/java/im/vector/app/features/crypto/verification/choose/VerificationChooseMethodFragment.kt
Outdated
Show resolved
Hide resolved
.../java/im/vector/app/features/crypto/verification/choose/VerificationChooseMethodViewModel.kt
Outdated
Show resolved
Hide resolved
android:layout_height="wrap_content" | ||
android:gravity="center_horizontal" | ||
android:orientation="horizontal" | ||
tools:visibility="visible"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tools:visibility
can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
vector/src/main/res/layout/item_positive_destrutive_buttons.xml
Outdated
Show resolved
Hide resolved
.../java/im/vector/app/features/crypto/verification/choose/VerificationChooseMethodViewModel.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good for me though I didn't correctly understand how to test and if is possible to test. I tried triggering some session verifications but I have not the same UI with simple "Accept"/"Decline" bottom sheet. So I am not sure this is the correct tests.
Not related, but I simply noticed for a while the top bar notification to trigger a verification is not displayed just after the first sign in. You need to restart the app to see it, I don't know if it was an intended change.
SonarCloud Quality Gate failed. |
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_marginEnd="16dp" | ||
tools:text="@string/action_decline" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK to use tools
but in this case, I think that positiveText
and destructiveText
should not be nullable. (see https://github.com/vector-im/element-android/pull/6328/files#diff-e03d15165cd3cc55c597e31b0494b0088b8ef56da1edb7468b870973d2948265R35)
This is not a blocker though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
Type of change
Content
Under certain condition, incoming verification are displayed in the UI without Accept/Decline button.
Making it impossible to continue the verification.
This PR fixes the case when a verification request is ignored because deemed too in the past or too in the future.
This computation is based on ageLocalTs, that is computed from the
unsigned.age
. The processor and the timeline display had different test, the timeline was accepting null age and not processor.To fix it, we now ensure that
ageLocalTs
is set even if nounsigned.age
in the synced event.I noticed that it was also possible to open the bottom sheet from typing on the verification tile and not on one of the button. I modified the bottom sheet to show accept/decline buttons when the request is not yet ready.
Motivation and context
Screenshots / GIFs
Tests
Tested against polyjuice for regressions.
Tested devices
Checklist