Skip to content

Don't display 'join room' screen while leaving a room. #2770

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 30, 2024
Merged

Don't display 'join room' screen while leaving a room. #2770

merged 3 commits into from
Apr 30, 2024

Conversation

jmartinesp
Copy link
Member

@jmartinesp jmartinesp commented Apr 30, 2024 •
edited
Loading

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Centralise the navigation based on membership in a single point in RoomFlowNode.
  • Don't display 'join room' screen while leaving a room.

Motivation and context

Fix annoying issue, deduplicate code.

Tests

  • From the room details screen, leave a room.
  • Check the 'join room' screen is not shown anymore (previously it was displayed for half a second).

Tested devices

  • Physical
  • Emulator
  • OS version(s): 14

Checklist

@jmartinesp jmartinesp marked this pull request as ready for review April 30, 2024 09:29
@jmartinesp jmartinesp requested a review from a team as a code owner April 30, 2024 09:29
@jmartinesp jmartinesp requested review from bmarty and removed request for a team April 30, 2024 09:29
bmarty
bmarty reviewed Apr 30, 2024
View reviewed changes
appnav/src/main/kotlin/io/element/android/appnav/room/RoomFlowNode.kt Outdated
if (info?.currentUserMembership == CurrentUserMembership.JOINED) {
if (info.isSpace) {
.map { it.getOrNull() }
.filterNotNull()
Copy link
Member

@bmarty bmarty Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the null case is a valid case (the room is not known, for instance following a permalink). With this change, I understand that the null case will not be handled anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, nice catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed in 59c9bad

Copy link
Member

Choose a reason for hiding this comment

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

Sadly there was no unit test to prevent the regression. We may want to extract the logic somewhere... (in the future)

@github-actions GitHub Actions
Copy link
Contributor

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/yRuVVi

@jmartinesp jmartinesp force-pushed the fix/jme/dont-display-join-room-screen-when-leaving-room branch from e5aba69 to 6010841 Compare April 30, 2024 09:42
@codecov Codecov
Copy link

codecov bot commented Apr 30, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (develop@c690b38). Click here to learn what that means.

Additional details and impacted files 
@@            Coverage Diff             @@
##             develop    #2770   +/-   ##
==========================================
  Coverage           ?   73.47%           
==========================================
  Files              ?     1517           
  Lines              ?    36418           
  Branches           ?     7012           
==========================================
  Hits               ?    26758           
  Misses             ?     5995           
  Partials           ?     3665

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jmartinesp
Don't display 'join room' screen while leaving a room.
00009d0 
Centralise the navigation based on membership in a single point in `RoomFlowNode`.
@jmartinesp
Restore null case
59c9bad
@jmartinesp jmartinesp force-pushed the fix/jme/dont-display-join-room-screen-when-leaving-room branch from 9941abb to 59c9bad Compare April 30, 2024 11:05
@jmartinesp
Copy link
Member Author

I don't really get why 889263e is needed. Maybe something went wrong when merging some other branch to develop?

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

bmarty
bmarty approved these changes Apr 30, 2024
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jmartinesp jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 30, 2024
@github-actions github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Apr 30, 2024
@jmartinesp jmartinesp merged commit 6d924c3 into develop Apr 30, 2024
18 of 19 checks passed
@jmartinesp jmartinesp deleted the fix/jme/dont-display-join-room-screen-when-leaving-room branch April 30, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmarty bmarty bmarty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jmartinesp @bmarty