Details

Reviewers

tomek
atul

Commits

rCOMM5231a4122892: [keyserver] Migration to reset all PRIVATE thread names

Summary

For all PRIVATE threads where the thread name is the member's username, this will clear out the thread name.

Once D7453 is widely deployed to clients, this will have the effect of allowing ENS resolution for PRIVATE thread names. For older clients without D7453, this would result in the threads being titled "just you".

To reduce the likelihood that a user encounters the "just you" case, I'll wait until we have a build with D7453 live in prod for a bit before landing this diff.

Depends on D7454

Test Plan

Tested the migration in my local environment. Confirmed it unsets the thread names correctly

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Apr 15 2023, 11:42 AM

Make sure the role is real so we don't pick up parent admins

keyserver/src/database/migration-config.js
327–333 ↗	(On Diff #25191)	I learned how craft an update with a join on MySQL via this article: https://www.mysqltutorial.org/mysql-update-join/ If there are `PRIVATE` thread with multiple `memberships`, this will clear the the username if the thread name matches any of the members' usernames. This should never happen since `PRIVATE` threads should have only one member

Harbormaster completed remote builds in B18385: Diff 25190.Apr 15 2023, 12:00 PM

ashoat published this revision for review.Apr 15 2023, 12:02 PM

I should either:

Figure out a way to gate this on codeVersion instead, and add a corresponding redux-persist migration to prevent native clients from having to get updated via the state check mechanism
Or generate a bunch of updateTypes.UPDATE_THREAD updates to update the thread names for all clients

Option 2 would probably be easier, and the cost should be low since most users will see at most one PRIVATE thread.

Harbormaster completed remote builds in B18386: Diff 25191.Apr 15 2023, 12:05 PM

Update to use updateThread utility so that updates are correctly created for all clients
Only reset thread names to the empty string if they are for SIWE users. For other users it won't affect behavior on new clients, but will break old clients

(I am still planning to defer landing this until the client code has been in place for some time.)

Harbormaster completed remote builds in B18424: Diff 25237.Apr 17 2023, 4:55 PM

tomek accepted this revision.Apr 17 2023, 11:39 PM

tomek added inline comments.

keyserver/src/database/migration-config.js
340–341 ↗	(On Diff #25237)	Are you sure we need left joins?

This revision is now accepted and ready to land.Apr 17 2023, 11:39 PM

ashoat added inline comments.Apr 18 2023, 4:36 AM

keyserver/src/database/migration-config.js
340–341 ↗	(On Diff #25237)	As opposed to other kinds of joins, or as opposed to selecting from multiple tables? As opposed to inner joins, it should make no difference because if either memberships or users tables don't match, then the `t.name = u.ethereum_address` condition will not be fulfilled. As opposed to multiple tables, my understanding is that the query engine should resolve this to the same behavior.

tomek added inline comments.Apr 18 2023, 4:41 AM

keyserver/src/database/migration-config.js
340–341 ↗	(On Diff #25237)	As opposed to inner joins. Not sure if it affects the performance, but regardless, if we don't want the rows that are matched by left and not by inner, we can be more explicit about it by using inner join - I think it is less confusing. Personally, I think that inner joins should be used by default and left joins only when there's a reason to do so.