fix_: persist left communities even for restored account #5174

kounkou · 2024-05-16T20:56:01Z

Description

This PR fixes #7858 by making sure left persisted communities are restored during the backup restore flow

Closes #7858

Screenshot

Since it's hard to make integration test that can simulate the delete of the data folder, I'll make a video to show that it works in principle.

Screencast.from.2024-05-16.13-51-32.webm

status-im-auto · 2024-05-16T20:59:07Z

Jenkins Builds

Click to see older builds (29)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✖️	`7c05b64`	#1	2024-05-16 20:59:07	~2 min	`tests`	📄`log`

✔️	`7c05b64`	#1	2024-05-16 21:00:53	~4 min	`linux`	📦`zip`
✔️	`7c05b64`	#1	2024-05-16 21:01:06	~4 min	`ios`	📦`zip`
✔️	`7c05b64`	#1	2024-05-16 21:02:12	~5 min	`android`	📦`aar`

✔️	`5ff3997`	#2	2024-05-16 21:07:36	~1 min	`android`	📦`aar`
✔️	`5ff3997`	#2	2024-05-16 21:08:43	~2 min	`ios`	📦`zip`
✔️	`5ff3997`	#2	2024-05-16 21:09:53	~3 min	`linux`	📦`zip`

✖️	`5ff3997`	#2	2024-05-16 21:43:51	~37 min	`tests`	📄`log`

✔️	`361bb04`	#3	2024-05-16 22:22:07	~2 min	`linux`	📦`zip`
✔️	`361bb04`	#3	2024-05-16 22:22:58	~3 min	`ios`	📦`zip`
✔️	`361bb04`	#3	2024-05-16 22:25:32	~5 min	`android`	📦`aar`

✖️	`361bb04`	#3	2024-05-16 22:55:28	~35 min	`tests`	📄`log`
✖️	`361bb04`	#5	2024-05-16 23:36:57	~36 min	`tests`	📄`log`

✔️	`0053415`	#4	2024-05-16 23:26:06	~1 min	`android`	📦`aar`
✔️	`0053415`	#4	2024-05-16 23:26:37	~2 min	`linux`	📦`zip`
✔️	`0053415`	#4	2024-05-16 23:27:35	~3 min	`ios`	📦`zip`

✔️	`0053415`	#6	2024-05-17 00:20:22	~43 min	`tests`	📄`log`

✔️	`c8b6b58`	#5	2024-05-18 01:53:37	~2 min	`linux`	📦`zip`
✔️	`c8b6b58`	#5	2024-05-18 01:54:06	~3 min	`android`	📦`aar`
✔️	`c8b6b58`	#5	2024-05-18 01:54:50	~3 min	`ios`	📦`zip`

✔️	`038a6b3`	#6	2024-05-18 02:00:12	~2 min	`android`	📦`aar`
✔️	`038a6b3`	#6	2024-05-18 02:00:28	~2 min	`linux`	📦`zip`
✔️	`038a6b3`	#6	2024-05-18 02:01:40	~3 min	`ios`	📦`zip`

✔️	`c8b6b58`	#7	2024-05-18 02:34:24	~43 min	`tests`	📄`log`

✔️	`038a6b3`	#8	2024-05-18 03:18:10	~43 min	`tests`	📄`log`

✔️	`a58f720`	#7	2024-05-20 21:08:46	~2 min	`android`	📦`aar`
✔️	`a58f720`	#7	2024-05-20 21:08:57	~2 min	`linux`	📦`zip`
✔️	`a58f720`	#7	2024-05-20 21:09:33	~3 min	`ios`	📦`zip`

✔️	`a58f720`	#9	2024-05-20 21:49:25	~42 min	`tests`	📄`log`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`ff1a93c`	#8	2024-05-21 19:22:42	~2 min	`linux`	📦`zip`
✔️	`ff1a93c`	#8	2024-05-21 19:22:58	~2 min	`android`	📦`aar`
✔️	`ff1a93c`	#8	2024-05-21 19:23:52	~3 min	`ios`	📦`zip`

✔️	`ff1a93c`	#10	2024-05-21 20:05:03	~44 min	`tests`	📄`log`

protocol/communities_messenger_test.go

jrainville

I had some questions

protocol/messenger_backup_test.go

protocol/messenger_communities.go

protocol/communities_messenger_test.go

MishkaRogachev

A few questions

protocol/communities/persistence.go

protocol/messenger_backup_test.go

This PR fixes #7858 by making sure left persisted communities are restored during the backup restore flow

osmaczko

Sorry for being late into the party 😞

Since newly introduced LeftCommunities seems to be a subset of DeletedCommunities I am not sure how this fixes the issue 🤔 Am I missing something here?

osmaczko · 2024-05-22T10:25:33Z

protocol/messenger_backup.go

+	Left    []*communities.Community
+	Deleted []*communities.Community


I don't think there is a semantic difference between Left and Deleted communities. I found the originating PR: #2639 and the issue: status-im/status-mobile#13198, which states: "Communities you left are not backed up/synced".

Newly introduced LeftCommunities seems to be a subset of DeletedCommunities:

LEFT:

query := communitiesBaseQuery + ` WHERE NOT c.Joined AND NOT c.spectated AND r.state != ?`

DELETED:

query := communitiesBaseQuery + ` WHERE NOT c.Joined AND (r.community_id IS NULL or r.state != ?)`

kounkou · 2024-05-22T17:27:27Z

Sorry for being late into the party 😞

Since newly introduced LeftCommunities seems to be a subset of DeletedCommunities I am not sure how this fixes the issue 🤔 Am I missing something here?

Hi @osmaczko, You are right, the LeftCommunities is a subset of DeletedCommunities, however it is confusing that we have DeletedCommunities since the condition r.community_id IS NULL is not trivial. If a communityId is NULL, it means it doesn't exist correct? However, if I leave a community, I should be able to become member again, which implies it still exists.
I saw you posted a comment in the other PR, and I am okay to remove my change all together if clarified there. My code has the advantage of having a clear query that is easy to understand and to reason about.

osmaczko · 2024-05-22T18:04:38Z

Sorry for being late into the party 😞
Since newly introduced LeftCommunities seems to be a subset of DeletedCommunities I am not sure how this fixes the issue 🤔 Am I missing something here?

Hi @osmaczko, You are right, the LeftCommunities is a subset of DeletedCommunities, however it is confusing that we have DeletedCommunities since the condition r.community_id IS NULL is not trivial. If a communityId is NULL, it means it doesn't exist correct? However, if I leave a community, I should be able to become member again, which implies it still exists. I saw you posted a comment in the other PR, and I am okay to remove my change all together if clarified there. My code has the advantage of having a clear query that is easy to understand and to reason about.

I understand the r.community_id IS NULL condition is causing confusion but since it is or'ed it shouldn't have impact. I would opt for reverting the changes (without test ofc), because having LeftCommunities & DeletedCommunities is even more confusing and also results in redundancy in backed-up communities. Ideally, the problematic condition should also be removed (backing-up community without id doesn't make sense) but let's wait for the response from the author.

kounkou · 2024-05-23T00:01:19Z

Wait @osmaczko, after second thought, actually that's surprising,
Before making my changes, I tested that performing a back up even manually didn't produce exepected result with the original code, and I had shared that during meetings we had.
Is it possible that I got unlucky and the test didn't pass that one time.
Let me peform testing of before the changes again before I can revert.

I did the test without current pr changes again and it fails, so somehow, these changes are needed

Screencast.from.2024-05-22.17-16-11.webm

I will do a deeper analysis on this issue.

My query should be :

query := communitiesBaseQuery + ` WHERE NOT c.Joined AND NOT c.spectated`

Because having the state to be anything but Pending join is not correct. Still investigating

This PR fixes #7858 by making sure left persisted communities are restored during the backup restore flow

kounkou linked an issue May 16, 2024 that may be closed by this pull request

Community -> Recovered account: community that user has left, is recovered in new installation status-im/status-desktop#7858

Closed

kounkou force-pushed the fix-persist_left_community branch 3 times, most recently from 361bb04 to 0053415 Compare May 16, 2024 23:24

kounkou commented May 16, 2024

View reviewed changes

protocol/communities_messenger_test.go Outdated Show resolved Hide resolved

kounkou self-assigned this May 17, 2024

kounkou requested review from MishkaRogachev, osmaczko and mprakhov May 17, 2024 00:26

jrainville reviewed May 17, 2024

View reviewed changes

protocol/messenger_backup_test.go Outdated Show resolved Hide resolved

protocol/messenger_communities.go Outdated Show resolved Hide resolved

protocol/communities_messenger_test.go Outdated Show resolved Hide resolved

kounkou force-pushed the fix-persist_left_community branch 2 times, most recently from c8b6b58 to 038a6b3 Compare May 18, 2024 01:57

kounkou requested a review from jrainville May 18, 2024 02:07

MishkaRogachev reviewed May 20, 2024

View reviewed changes

protocol/communities/persistence.go Outdated Show resolved Hide resolved

protocol/communities/persistence.go Show resolved Hide resolved

kounkou force-pushed the fix-persist_left_community branch from 038a6b3 to a58f720 Compare May 20, 2024 21:06

kounkou requested a review from MishkaRogachev May 20, 2024 21:07

jrainville approved these changes May 21, 2024

View reviewed changes

protocol/messenger_backup_test.go Outdated Show resolved Hide resolved

MishkaRogachev approved these changes May 21, 2024

View reviewed changes

fix_: persist left communities even for restored account

ff1a93c

This PR fixes #7858 by making sure left persisted communities are restored during the backup restore flow

kounkou force-pushed the fix-persist_left_community branch from a58f720 to ff1a93c Compare May 21, 2024 19:19

kounkou merged commit 133ad09 into develop May 21, 2024
12 checks passed

kounkou deleted the fix-persist_left_community branch May 21, 2024 21:01

This was referenced May 21, 2024

chore(status-go): bump status-go to get left communities persistence … status-im/status-desktop#14875

Closed

chore(status-go): bump status-go to get left communities persistence … status-im/status-desktop#14876

Merged

osmaczko reviewed May 22, 2024

View reviewed changes

chaitanyaprem pushed a commit that referenced this pull request May 28, 2024

fix_: persist left communities even for restored account (#5174)

f97e395

This PR fixes #7858 by making sure left persisted communities are restored during the backup restore flow

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix_: persist left communities even for restored account #5174

fix_: persist left communities even for restored account #5174

kounkou commented May 16, 2024 •

edited

Loading

status-im-auto commented May 16, 2024 •

edited

Loading

jrainville left a comment

MishkaRogachev left a comment

osmaczko left a comment

osmaczko May 22, 2024

kounkou commented May 22, 2024

osmaczko commented May 22, 2024

kounkou commented May 23, 2024 •

edited

Loading

		Left []*communities.Community
		Deleted []*communities.Community

fix_: persist left communities even for restored account #5174

fix_: persist left communities even for restored account #5174

Conversation

kounkou commented May 16, 2024 • edited Loading

Description

Screenshot

status-im-auto commented May 16, 2024 • edited Loading

Jenkins Builds

jrainville left a comment

Choose a reason for hiding this comment

MishkaRogachev left a comment

Choose a reason for hiding this comment

osmaczko left a comment

Choose a reason for hiding this comment

osmaczko May 22, 2024

Choose a reason for hiding this comment

kounkou commented May 22, 2024

osmaczko commented May 22, 2024

kounkou commented May 23, 2024 • edited Loading

kounkou commented May 16, 2024 •

edited

Loading

status-im-auto commented May 16, 2024 •

edited

Loading

kounkou commented May 23, 2024 •

edited

Loading