[Device management] Verify another session (PSG-722) #7156
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mnaturel
changed the title
mnaturel
requested review from
a team and
fedrunov
and removed request for
a team
bmarty
reviewed
Sep 16, 2022
|
|
||
| sealed class SessionOverviewViewEvent : VectorViewEvents { | ||
| data class SelfVerification(val session: Session) : SessionOverviewViewEvent() | ||
| data class ShowVerifyCurrentSession(val session: Session) : SessionOverviewViewEvent() |
Member
There was a problem hiding this comment.
Just a small remark: please remove the session class member.
Contributor
Author
There was a problem hiding this comment.
Yes thanks, I removed the argument.
mnaturel
force-pushed
the
feature/mna/device-manager-verify-current-session
branch
from
d578dbe to
4f3df2c
Compare
mnaturel
force-pushed
the
feature/mna/device-manager-verify-other-session
branch
from
6c8cd04 to
ef34f69
Compare
15 tasks
bmarty
approved these changes
Sep 19, 2022
| updateEntryDetails(state.deviceId) | ||
| if (state.deviceInfo is Success) { | ||
| renderSessionInfo(state.isCurrentSession, state.deviceInfo.invoke()) | ||
| renderSessionInfo(state.isCurrentSession, state.deviceInfo.invoke(), state.isCurrentSessionTrusted) |
Member
There was a problem hiding this comment.
Nit: Maybe passing the state as a single argument could be simpler, but maybe not, because of the type check of state.deviceInfo
Contributor
Author
There was a problem hiding this comment.
I moved all the logic into a updateSessionInfo() method. See f54955d
bmarty
approved these changes
Sep 19, 2022
Member
bmarty
left a comment
bmarty
left a comment
There was a problem hiding this comment.
One tiny last remark that you could simply ignore, else LGTM!
| ) | ||
| views.sessionOverviewInfo.render(infoViewState, dateFormatter, drawableProvider, colorProvider) | ||
| } else { | ||
| hideSessionInfo() |
Member
There was a problem hiding this comment.
For clarity/parity, I would then remove fun hideSessionInfo() and just write:
Suggested change
| hideSessionInfo() | |
| views.sessionOverviewInfo.isVisible = false |
Contributor
Author
There was a problem hiding this comment.
I will make this change in a following PR.
mnaturel
force-pushed
the
feature/mna/device-manager-verify-current-session
branch
from
4f3df2c to
963c5f8
Compare
mnaturel
force-pushed
the
feature/mna/device-manager-verify-other-session
branch
from
f54955d to
2eb0391
Compare
|
SonarCloud Quality Gate failed. |
added 8 commits
September 20, 2022 10:46
mnaturel
force-pushed
the
feature/mna/device-manager-verify-other-session
branch
from
2eb0391 to
943ec7e
Compare
mnaturel
force-pushed
the
feature/mna/device-manager-verify-current-session
branch
from
963c5f8 to
54a4dc7
Compare
Base automatically changed from
feature/mna/device-manager-verify-current-session
to
develop
September 22, 2022 09:46
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of change
Content
Handling click on the "Verify" button in the session overview screen for other session than the current one.
Motivation and context
Closes #7143
Screenshots / GIFs
No UI changed.
Tests
Tested devices
Checklist