Skip to content

Comments

feat: introduce communities settings#2500

Closed
0x-r4bbit wants to merge 1 commit intostatus-im:developfrom
0x-r4bbit:chamvp
Closed

feat: introduce communities settings#2500
0x-r4bbit wants to merge 1 commit intostatus-im:developfrom
0x-r4bbit:chamvp

Conversation

@0x-r4bbit
Copy link
Member

@0x-r4bbit 0x-r4bbit commented Jan 24, 2022

This commit introduces new communities settings that are needed for the
upcoming Community History Archive support.

Namely, we need to persist whether community owners will seed message
history archives and whether community members are interested in
fetching such archives.

The settings are defined as follows:

  1. community_id - The id of the community the settings belong to
  2. message_archive_seeding_enabled - Whether message history archives
    will be created and seeded for this community
  3. message_archive_fetching_enabled - Whether message history archives
    will fetched for this community

The data is persisted in a newly introduced communities_settings
table.

Furthermore, the RPC API that returns Settings for the logged in
account will now include a communities-settings field of type:
[]CommunitySettings.

The community settings in the database will be added, updated an deleted
depending whether the user joins, leaves or edits the community in
question.

Tasks

  • Write tests
  • Correct formatting
  • Migration script to add community settings for existing communities

Closes #2495

@ghost
Copy link

ghost commented Jan 24, 2022

Pull Request Checklist

  • Have you updated the documentation, if impacted (e.g. docs.status.im)?
  • Have you tested changes with mobile?
  • Have you tested changes with desktop?

@status-im-auto
Copy link
Member

status-im-auto commented Jan 24, 2022

Jenkins Builds

Click to see older builds (6)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 05ddbfc #1 2022-01-24 13:17:44 ~2 min linux 📦zip
✔️ 05ddbfc #1 2022-01-24 13:18:59 ~3 min ios 📦zip
✔️ 05ddbfc #1 2022-01-24 13:19:18 ~3 min android 📦aar
✔️ 4b4896d #2 2022-01-24 14:20:58 ~1 min ios 📦zip
✔️ 4b4896d #2 2022-01-24 14:21:18 ~2 min linux 📦zip
✔️ 4b4896d #2 2022-01-24 14:23:21 ~4 min android 📦aar
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 4c6b75f #3 2022-01-24 19:41:45 ~1 min ios 📦zip
✔️ 4c6b75f #3 2022-01-24 19:41:45 ~1 min linux 📦zip
✔️ 4c6b75f #3 2022-01-24 19:43:05 ~3 min android 📦aar
✔️ 7695cd4 #4 2022-01-24 19:50:59 ~1 min ios 📦zip
✔️ 7695cd4 #4 2022-01-24 19:51:19 ~2 min linux 📦zip
✔️ 7695cd4 #4 2022-01-24 19:52:11 ~2 min android 📦aar

)

communitiesSettings, err := db.GetCommunitiesSettings()
s.CommunitiesSettings = communitiesSettings
Copy link
Member Author

Choose a reason for hiding this comment

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

Since communities_settings live in a separate table, I'm explicitly querying/assigning it here.

Let me know if this should not happen here and whether we prefer a separate RPC API so clients can request that data.

CommunityID string `json:"communityId"`
MessageArchiveFetchingEnabled bool `json:"messageArchiveFetchingEnabled"`
MessageArchiveSeedingEnabled bool `json:"messageArchiveSeedingEnabled"`
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This might not be the proper place to put this type.

Eventually, we probably need a type for enabling/disabling community history archive support in the CLI as well. Might as well leverage this type for that, which is why I put it here.

Happy to put it somewhere else.

CommunityID: communityID.String(),
MessageArchiveFetchingEnabled: true,
MessageArchiveSeedingEnabled: false,
}
Copy link
Member Author

Choose a reason for hiding this comment

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

These are the defaults when joining a community.

If could also introduce a GetDefaultCommunitySettings API if you prefer.

ImageBx int `json:"imageBx"`
ImageBy int `json:"imageBy"`
MessageArchiveSeedingEnabled bool `json:"messageArchiveSeedingEnabled,omitempty"`
MessageArchiveFetchingEnabled bool `json:"messageArchiveFetchingEnabled,omitempty"`
Copy link
Member Author

Choose a reason for hiding this comment

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

I made those two omitempty so we can potentially put these changes into production without having the need for desktop and/or mobile to do anything in particular.

@0x-r4bbit
Copy link
Member Author

Have you updated the documentation, if impacted (e.g. docs.status.im)?

There's a pending PR for a protocol spec addition that eventually needs these changes: status-im/specs#164

This commit introduces new communities settings that are needed for the
upcoming Community History Archive support.

Namely, we need to persist whether community owners will seed message
history archives and whether community members are interested in
fetching such archives.

The settings are defined as follows:

1. `community_id` - The id of the community the settings belong to
2. `message_archive_seeding_enabled` - Whether message history archives
   will be created and seeded for this community
3. `message_archive_fetching_enabled` - Whether message history archives
will fetched for this community

The data is persisted in a newly introduced `communities_settings`
table.

Furthermore, the RPC API that returns `Settings` for the logged in
account will now include a `communities-settings` field of type:
`[]CommunitySettings`.

The community settings in the database will be added, updated an deleted
depending whether the user joins, leaves or edits the community in
question.

Closes status-im#2495
@0x-r4bbit
Copy link
Member Author

The linting error caused in this PR: https://ci.status.im/blue/organizations/jenkins/status-go%2Fprs%2Ftests/detail/PR-2500/4/pipeline#step-60-log-3 seems to be unrelated to my changes.

@0x-r4bbit
Copy link
Member Author

As discussed offline with @cammellos, introduction of communities_settings for existing communities (prior to this PR/commit) can possibly be done in an SQL migration script.

I'll give that a spin, so please don't merge this yet.

return description, nil
}

func (c *CreateCommunity) ToCommunitySettings(publicKey string) (params.CommunitySettings, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Is an error expected? if not, it can be removed from the return parameters

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not, wanted to stay consistent. But happy to change this!

return "", err
}

func (db *Database) SaveCommunitySettings(communitySettings params.CommunitySettings) error {
Copy link
Member

Choose a reason for hiding this comment

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

In protocol/communities/persistence.go live all the functions related to communities CRUD operations. What do you think of having these community settings functions live there too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to move it over!

Main reason I put them here was because I consider them more "settings" related to the account. Like.. we could also call the entire thing "node_settings" if we want (once we get to actually supporting all the settings like throttling etc). In that sense it wouldn't necessarily be community specific anymore.

But yea, I totally see where you're coming from, so I'll move it over for now. We can still migrate it to somewhere else once this stuff gets broader responsibility.

@@ -0,0 +1,5 @@
CREATE TABLE communities_settings (
community_id TEXT PRIMARY KEY ON CONFLICT REPLACE,
Copy link
Member

Choose a reason for hiding this comment

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

In protocol/migrations/sqlite/1605075346_add_communities.up.sql the community_id is defined as
id BLOB NOT NULL PRIMARY KEY ON CONFLICT REPLACE. Consider using the same data type (blob)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call!

Comment on lines +882 to +885
if err != nil {
return err
}
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if err != nil {
return err
}
return nil
return err

Comment on lines +563 to +564
communitiesSettings, err := db.GetCommunitiesSettings()
s.CommunitiesSettings = communitiesSettings
Copy link
Member

@richard-ramos richard-ramos Jan 26, 2022

Choose a reason for hiding this comment

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

If there's an err, it might end up being overwritten by a succesful db.getCommunitiesSetting() response

Suggested change
communitiesSettings, err := db.GetCommunitiesSettings()
s.CommunitiesSettings = communitiesSettings
if err != nil {
return s, err
}
communitiesSettings, err := db.GetCommunitiesSettings()
s.CommunitiesSettings = communitiesSettings

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!!

@0x-r4bbit
Copy link
Member Author

Closing in favour of #2574

@0x-r4bbit 0x-r4bbit closed this Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Introduce flag to enable/disable message history archive support in communities

3 participants