Fixes room not being in space after upgrade #6200

ericdecanini · 2022-05-31T09:36:19Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Fixes room not being in space after upgrade and possible duplications in Explore Rooms

Motivation and context

Closes #4133

Screenshots / GIFs

dedupe.mp4

Tests

In a space, go to a room with two of your test accounts and use the /upgraderoom 7 to upgrade the room from 9 to 7 (yes this still counts as an upgrade). Don't automatically reinvite users
On your other test account, ensure that you still see the room in the same space but going into it shows you that it's been upgraded
Invite your account and see that the room disappears and gets replaced by the invite
After accepting the invite see that in explore rooms you no longer see the pre-upgrade room

Tested devices

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

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

ouchadam · 2022-05-31T10:11:26Z

...or/src/main/java/im/vector/app/features/home/room/detail/upgrade/UpgradeRoomViewModelTask.kt

@@ -84,8 +84,6 @@ class UpgradeRoomViewModelTask @Inject constructor(
 //                                autoJoin = currentInfo.autoJoin ?: false,
                                suggested = currentInfo.suggested
                        )
-
-                        parentSpace.removeChildren(params.roomId)


do we know why this was originally added?

I would assume the room id is changed after upgrade and we're cleaning the legacy room 🤔

I would guess this was the first attempt at removing the duplication, and though it worked for that purpose, you would no longer see the pre-upgraded room in the space but you'd see it in Home

This could be valuable for other cases, some counters could use spaces children number or some check (if space is empty or not), so absence of removal could cause some side effects.

@BillCarsonFr you added this line, maybe you could verify if removing it could cause any problems?

Yes in the original code when migrating a room, we were removing the old room from the space and then add the new one. The annoying side effect is that people that haven't yet migrated will then see the room as not being in the space anymore (it will probably jump back to home).
Now we want to keep both old & new as part of the migration process, and to have the UI smarter and detect duplicate rooms and only show the new one.
Like that new comers will only see the new room, and existing users will see the old one in the space with the CTA to join the migrated one.

I was thinking about some places, where we could get rooms without applying this filter, like getting space''s room count or something like that. 🤔

github-actions · 2022-05-31T10:41:40Z

Unit Test Results

128 files - 22 128 suites - 22 2m 15s ⏱️ -15s
223 tests - 16 223 ✔️ - 16 0 💤 ±0 0 ❌ ±0
738 runs - 62 738 ✔️ - 62 0 💤 ±0 0 ❌ ±0

Results for commit 36fc2fb. ± Comparison against base commit d586f64.

♻️ This comment has been updated with latest results.

ouchadam

I'm not tooo familiar with Spaces, codewise this looks good to me! 💯

ouchadam · 2022-05-31T11:08:23Z

vector/src/main/java/im/vector/app/features/spaces/explore/SpaceDirectoryController.kt

@@ -209,4 +211,7 @@ class SpaceDirectoryController @Inject constructor(
            }
        }
    }
+
+    private fun SpaceChildInfo.isUpgradedRoom(data: SpaceDirectoryState) =
+            data.knownRoomSummaries.any { it.roomId == childRoomId && it.versioningState != VersioningState.NONE }


super nit: could be a little easier on the brain to lift the isUpgraded = it.versioningState != VersioningState.NONE to an extension on the VersioningState

has the benefit of keeping the conditions here positive and future proofs this logic from any possible new versioning states which aren't upgrades

I agree, good shout!

fedrunov · 2022-06-06T07:41:08Z

vector/src/main/java/im/vector/app/features/spaces/explore/SpaceDirectoryController.kt

@@ -110,6 +110,7 @@ class SpaceDirectoryController @Inject constructor(
                    ?.filter {
                        it.parentRoomId == (data.hierarchyStack.lastOrNull() ?: data.spaceId)
                    }
+                    ?.filterNot { it.isUpgradedRoom(data) }


it will filter out rooms with UPGRADED_ROOM_NOT_JOINED VersioningState, is it intentional? If room was upgraded, but user haven't joined upgraded room yet, what user will see? We change state from NONE to UPGRADED_ROOM_NOT_JOINED on TombStone event and so it could happen while user is on a background and doesn't interact with the app.

This is intentional yeah, this only applies to the old version of upgraded rooms, so them not being active there's no use having them shown in Explore Rooms

Just to clarify, we should filter out if the room is an old version of a room that is also in the space (we want to avoid apparant duplicates in the list).

So, when room is upgraded, everyone in the room gets tombstone event and state is changed to UPGRADED_ROOM_NOT_JOINED until they will join new room. isUpgradedRoom will return true and so such users won't see room in a list. Shouldn't we hide it only after users will join new Room?

We should check what web is doing and do the same. What we don't want is for the user to see a duplicated room (for the user it's pretty much the same, after all room upgrade is just a technical thing).
I guess that in the case you mentionned, I would want as a user to see only one room and probably that I joined?
So that mean we show the old room? or the new one but with an indicator that I joined the old version?
anyhow we should align with web and amend later with better behavior if needed

We already have a de-duplication mechanism for upgraded room in the room list. We need similar thing in the explore screen

I can see web doing this same as this PR. The pre-upgraded room is kept in the space

But you don't keep it, you filter out any room with UPGRADED_ROOM_NOT_JOINED, so it's different from web, no?

I mean keep in the sense that it's not deleted from the space in the db, it's just filtered out in the UI

fedrunov · 2022-06-06T07:44:00Z

...or/src/main/java/im/vector/app/features/home/room/detail/upgrade/UpgradeRoomViewModelTask.kt

@@ -84,8 +84,6 @@ class UpgradeRoomViewModelTask @Inject constructor(
 //                                autoJoin = currentInfo.autoJoin ?: false,
                                suggested = currentInfo.suggested
                        )
-
-                        parentSpace.removeChildren(params.roomId)


This could be valuable for other cases, some counters could use spaces children number or some check (if space is empty or not), so absence of removal could cause some side effects.

@BillCarsonFr you added this line, maybe you could verify if removing it could cause any problems?

…to bugfix/eric/upgrade-room-deduplication

…e-room-deduplication # Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/model/VersioningState.kt

BillCarsonFr

I noticed that this change is not consistent with web (did some tests).
Could you please sync with web and see why they are not doing it?
element-hq/element-meta#460
We should be consistent.

ericdecanini · 2022-07-01T14:59:18Z

I noticed that this change is not consistent with web (did some tests). Could you please sync with web and see why they are not doing it? vector-im/element-meta#460 We should be consistent.

We have this issue to decide and fix what should happen with regards to the room showing after an upgrade. This PR focuses on fixing the orphaning (issue you linked) and deduplication of the room after an upgrade. I think we should keep the focus of this PR on only that and any other changes, we address it in this issue below. This will also give us enough time to fix it for both web and android and gain parity again

#6411

BillCarsonFr · 2022-07-01T15:45:03Z

Is it possible to add a unit/integration test?

sonarcloud · 2022-07-04T10:27:45Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

28.6% Coverage
0.0% Duplication

Fixes upgraded room duplication

af4f8d0

ericdecanini added the PR-Small PR with less than 20 updated lines label May 31, 2022

ouchadam reviewed May 31, 2022

View reviewed changes

ericdecanini marked this pull request as draft May 31, 2022 10:41

ericdecanini added 2 commits May 31, 2022 12:53

Fixes room filter not including UPGRADE_ROOM_NOT_JOINED

8d93044

Adds changelog file

01b7395

ericdecanini marked this pull request as ready for review May 31, 2022 11:00

ericdecanini requested review from ouchadam and fedrunov May 31, 2022 11:00

ouchadam approved these changes May 31, 2022

View reviewed changes

ouchadam reviewed May 31, 2022

View reviewed changes

Adds isUpgraded method to VersioningState

f6b0e8d

fedrunov suggested changes Jun 6, 2022

View reviewed changes

ericdecanini requested review from BillCarsonFr and fedrunov June 7, 2022 10:39

Merge remote-tracking branch 'origin/bugfix/eric/fix-upgrade-room' in…

36fc2fb

…to bugfix/eric/upgrade-room-deduplication

Base automatically changed from bugfix/eric/fix-upgrade-room to develop June 17, 2022 08:55

Merge remote-tracking branch 'origin/develop' into bugfix/eric/upgrad…

e87d970

…e-room-deduplication # Conflicts: # matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/model/VersioningState.kt

fedrunov approved these changes Jun 29, 2022

View reviewed changes

BillCarsonFr requested changes Jun 30, 2022

View reviewed changes

ericdecanini requested a review from BillCarsonFr July 1, 2022 14:59

Adds VersioningStateTest

654eede

BillCarsonFr approved these changes Jul 20, 2022

View reviewed changes

ericdecanini merged commit 38ba61f into develop Jul 20, 2022

ericdecanini deleted the bugfix/eric/upgrade-room-deduplication branch July 20, 2022 10:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes room not being in space after upgrade #6200

Fixes room not being in space after upgrade #6200

ericdecanini commented May 31, 2022 •

edited

Loading

ouchadam May 31, 2022

ericdecanini May 31, 2022

fedrunov Jun 6, 2022

BillCarsonFr Jun 6, 2022

fedrunov Jun 6, 2022

github-actions bot commented May 31, 2022 •

edited

Loading

ouchadam left a comment

ouchadam May 31, 2022

ericdecanini Jun 3, 2022

fedrunov Jun 6, 2022

ericdecanini Jun 6, 2022

BillCarsonFr Jun 6, 2022

fedrunov Jun 6, 2022

BillCarsonFr Jun 6, 2022

BillCarsonFr Jun 6, 2022

ericdecanini Jun 7, 2022

fedrunov Jun 16, 2022

ericdecanini Jun 28, 2022

fedrunov Jun 6, 2022

BillCarsonFr left a comment

ericdecanini commented Jul 1, 2022

BillCarsonFr commented Jul 1, 2022 •

edited

Loading

sonarcloud bot commented Jul 4, 2022

Fixes room not being in space after upgrade #6200

Fixes room not being in space after upgrade #6200

Conversation

ericdecanini commented May 31, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 31, 2022 • edited Loading

Unit Test Results

ouchadam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillCarsonFr left a comment

Choose a reason for hiding this comment

ericdecanini commented Jul 1, 2022

BillCarsonFr commented Jul 1, 2022 • edited Loading

sonarcloud bot commented Jul 4, 2022

ericdecanini commented May 31, 2022 •

edited

Loading

github-actions bot commented May 31, 2022 •

edited

Loading

BillCarsonFr commented Jul 1, 2022 •

edited

Loading