Page MenuHomePhabricator

[keyserver/lib] Update the change_role message spec to include a role name field in the DB
ClosedPublic

Authored by rohan on Jul 14 2023, 1:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 5, 7:38 AM
Unknown Object (File)
Fri, Apr 5, 7:38 AM
Unknown Object (File)
Fri, Apr 5, 7:38 AM
Unknown Object (File)
Fri, Apr 5, 7:38 AM
Unknown Object (File)
Fri, Apr 5, 7:38 AM
Unknown Object (File)
Fri, Apr 5, 7:38 AM
Unknown Object (File)
Fri, Apr 5, 7:38 AM
Unknown Object (File)
Fri, Apr 5, 7:38 AM
Subscribers

Details

Summary

While writing the code to edit/delete roles, I noticed that the message spec for the change_roles message type causes the app to crash if a role is deleted. The reason for this is when a user's role
is changed, we write the following information to the DB in the content field:

{"userIDs":["83813"],"newRole":"84094"}

To generate the robotext for this message in chat (so we can show users something like 'jack assigned max the "Moderators" role'), we introspect into threadInfo.roles and try to access the role info using the newRole ID. When a role is deleted, we delete it from the database, and so the threadInfo will no longer have this role info and the app will crash. To resolve this, we should now store the roleName associated with the role change in the DB to use as a fallback option.

We want to continue trying to access the role name via the threadInfo (since older clients will not have this roleName field), but fallback onto it if needed. The reason this should not cause a problem is that older clients are guaranteed to have the role stored in threadInfo since at the moment, only Members and Admins are roles on Comm, and neither are deletable.

As for the code, the following is done:

If the client has the newest code version, show them the robotext accessing the role either through threadInfo or the content field as a fallback.

If the client does not, then we just shim the message.

Depends on D8478

Test Plan

Checked the before and after to verify that the new field was inserted into the DB when a role is changed. Then, confirmed that messages are shimmed successfully if my client's version is not meeting the minimum code version. Lastly, made sure that the app continues to display role names in the robotext even if it is deleted.

content field of the database when changing a user role (before):

Before (DB).png (140×2 px, 124 KB)

content field of the database when changing a user role (after):

After (DB).png (264×2 px, 215 KB)

Generated robotext for clients that do not meet the minimum code verison:

Shimmed Change Role.png (2×1 px, 1 MB)

Robotext for clients meeting the minimum code version with an existing custom role:

Existing Role - Unshimmed.png (2×1 px, 1 MB)

Robotext for clients meeting the minimum code version after the role has been deleted:

Deleted Role - Unshimmed.png (2×1 px, 1 MB)

For reference, the error I would get opening the thread after the role was deleted before this diff:

Simulator Screen Shot - iPhone 14 Pro - 2023-07-13 at 12.48.54.png (2×1 px, 259 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan edited the test plan for this revision. (Show Details)
rohan edited the test plan for this revision. (Show Details)
rohan attached a referenced file: F636036: After (DB).png. (Show Details)
rohan attached a referenced file: F636035: Before (DB).png. (Show Details)
rohan edited the test plan for this revision. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 14 2023, 1:40 PM
Harbormaster failed remote builds in B20937: Diff 28691!
rohan edited the test plan for this revision. (Show Details)

Update comment and make types read-only (they weren't before)

lib/shared/messages/change-role-message-spec.js
184 ↗(On Diff #28692)

I'm not sure if this is the right constant to use here / what to do prior to landing.

Before landing, should I replace it like usual with current code version + 2 to include public and staff releases Or is that part of the release process now and I should leave it as is?

rohan published this revision for review.Jul 14 2023, 1:46 PM

Will re-run CI once the issues are resolved

lib/shared/messages/change-role-message-spec.js
133 ↗(On Diff #28692)

What's the || null for? Typically ?? null is preferred is you really want to coerce to null, but I'm not clear on why you want to do that

136 ↗(On Diff #28692)

?? instead of ||

184 ↗(On Diff #28692)

Or is that part of the release process now and I should leave it as is?

That's the idea, but it helps to ping @atul to give him a heads-up

197 ↗(On Diff #28692)

Should we make this look more like the result of the robotext function? Is it possible to factor out the assigned ${affectedUsers} the \"${roleName}\" role part of the robotext method and call it here?

Worth noting that there are some nuances here for converting from`EntityText` to a string since it requires ENS resolution. Some ideas:

  1. Call entityTextToRawString and accept that ENS resolution would not occur
  2. Use "some members" or "a member" instead of affectedUsers for this case, so that ENS resolution doesn't matter. Then you can call entityTextToRawString without worrying
  3. (Best but hardest) call getEntityTextAsString and do ENS resolution, but first you'll need to convert shimUnsupportedMessageInfo and shimUnsupportedRawMessageInfos into async functions
lib/shared/messages/change-role-message-spec.js
197 ↗(On Diff #28692)

I thought the idea was that older clients won't have access to the new roleName field in the first place, so we'd have to shim the message. The original way we're getting the roleName is by accessing threadInfo.roles[messageInfo.newRole].name, but I took a look at the message-spec and it doesn't look like there's a way we can access params to get the threadInfo.

TLDR of that is I don't think we can access the role name for older clients with just the messageInfo.

Unless maybe I'm misundestanding the problem with 'older clients', and accessing messageInfo.roleName here won't actually be an issue

lib/shared/messages/change-role-message-spec.js
197 ↗(On Diff #28692)

I think shimUnsupportedMessageInfo gets called on the keyserver, not on clients

lib/shared/messages/change-role-message-spec.js
197 ↗(On Diff #28692)

Ah yeah good point

Waiting for review on D8540 to proceed and figure out how I should proceed with the feedback

Address feedback (option #2)

Shimmed messages:

Simulator Screen Shot - iPhone 14 Pro - 2023-07-26 at 12.52.41.png (2×1 px, 265 KB)

Unshimmed messages:

Simulator Screen Shot - iPhone 14 Pro - 2023-07-26 at 12.53.28.png (2×1 px, 268 KB)

ashoat requested changes to this revision.Jul 27 2023, 6:36 PM

Option #2 makes sense to me. The shimmed text isn't really something we should be spending a whole ton of time on

Requesting changes for two things I didn't notice in my last review (sorry!)

lib/shared/message-utils.js
636 ↗(On Diff #29067)

Is the backslash necessary here? Are we sure that it doesn't get rendered to the screen?

I see you're just moving this... I guess I may have introduced an error when I refactored this to use EntityText. Can you check if the backslashes get rendered to the screen?

lib/shared/messages/change-role-message-spec.js
139–142 ↗(On Diff #29067)

Can we safely assert this? What happens if a role gets deleted, and the user attempts to view an old message about that role?

When we introduce an invariant, we should be confident that it will never get triggered.

This revision now requires changes to proceed.Jul 27 2023, 6:36 PM
lib/shared/message-utils.js
636 ↗(On Diff #29067)

Yeah the backslash is needed to escape the " character, since we want to display something like you assigned jake the "Moderators" role (change was made last month in D8142).

They won't get rendered to the screen since they're escaped, this is what it looks like

simulator_screenshot_CE0C5969-92AB-4F09-87DA-FA1B4B133A91.png (2×1 px, 270 KB)

lib/shared/messages/change-role-message-spec.js
139–142 ↗(On Diff #29067)

Yeah we should be able to assert this with no problem, since we considered two cases:

  1. Currently, the only roles are Members and Admins (neither can be deleted, so we can ensure that for all old role change messages, the role name will be in threadInfo.
  2. For newer roles created (after this stack lands), they will also be available in the threadInfo. Once deleted, the newly introduced roleName field in the change role message info will be the 'fallback' option.

Just to be safe though, since constructChangeRoleEntityText already allows roleName to be optional for the case where we're shimming it, I can remove the invariant and pass in the roleName as-is. I expect it will always exist, but it'd be good to avoid an unnecessary crash

Minor comments, but please address before landing!

lib/shared/message-utils.js
636 ↗(On Diff #29067)

I can confirm that the backslashes don't appear in the string: {F662195}

That said, I don't think they're necessary. Can you clarify why you think the " character needs to be escaped? If testing confirms that things work fine without the escaping, please remove the backslashes before landing!

lib/shared/messages/change-role-message-spec.js
139–142 ↗(On Diff #29067)

Ah... what I was missing is that it's not possible to delete one of the "legacy" roles. So we should never get into a situation where an old-format message refers to a role that isn't in threadInfo.roles.

That said, I agree that this would be a good idea:

Just to be safe though, since constructChangeRoleEntityText already allows roleName to be optional for the case where we're shimming it, I can remove the invariant and pass in the roleName as-is. I expect it will always exist, but it'd be good to avoid an unnecessary crash

This revision is now accepted and ready to land.Jul 28 2023, 8:45 AM

Remove backslash (already addressed the feedback of removing the invariant in the prior revision)

This revision was landed with ongoing or failed builds.Jul 31 2023, 6:15 PM
This revision was automatically updated to reflect the committed changes.