[keyserver] Migration to add Voiced role to announcement subchannels
AbandonedPublic
Actions

Authored by rohan on Nov 10 2023, 9:54 AM.

Details

Reviewers

atul
ginsu
ashoat

Summary

I think there's one announcement subchannel in production, so I need a migration to add the Voiced role to this channel. I've made it a general migration that should cover all announcement subchannels in case there have been new ones created since I began working on this task.

Depends on D9804

Test Plan

Ran the migration on announcement subchannels created on master (without Voiced), and then confirmed the roles were created by inspecting MariaDB. Also ran this query to check that the roles were created for the correct channels:

SELECT * FROM threads WHERE id IN (SELECT thread FROM roles WHERE name='Voiced')

I only got results back of threads that were announcement channels (both open and secret)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Nov 10 2023, 9:54 AM

Herald added subscribers: will, tomek, ashoat. · View Herald TranscriptNov 10 2023, 9:54 AM

rohan added a child revision: D9805: [keyserver] Update changeRoleThreadQuery to use the Voiced role for announcement channels.Nov 10 2023, 9:55 AM

rohan edited the test plan for this revision. (Show Details)

rohan added inline comments.Nov 10 2023, 9:59 AM

keyserver/src/database/migration-config.js

694–710

I thought it was safer to batch the role insertions, but not really sure if it's necessary at all so I can simplify this if needed to just

const newRows = [];
for (const threadID of threads) {
  const id = ids.shift();
  const time = Date.now();
  
   newRows.push([id, threadID, voicedRoleName, permissionsBlob, time]);
}

const query = SQL`
      INSERT INTO roles (id, thread, name, permissions, creation_time)
      VALUES ${newRows}
`;
await dbQuery(query);

Harbormaster completed remote builds in B24001: Diff 33075.Nov 10 2023, 10:13 AM

rohan requested review of this revision.Nov 10 2023, 10:13 AM

rohan removed a child revision: D9805: [keyserver] Update changeRoleThreadQuery to use the Voiced role for announcement channels.Nov 10 2023, 10:42 AM

rohan added a child revision: D9827: [keyserver/lib/native] Introduce VOICED_IN_ANNOUNCEMENT_CHANNELS thread permission.Nov 10 2023, 11:30 AM

ashoat requested changes to this revision.Nov 10 2023, 12:56 PM

ashoat added inline comments.

keyserver/src/database/migration-config.js
684	This is confusing to me. It looks like you're creating one new `roles` ID for each of the roles in `rolePermissions`, but I don't follow why you're doing this. Won't you need more `ids` if `threads` is longer? Why do you need IDs for the already-existing `roles` rows like `Members`?
694–710	I think it's probably good that you're batching them

This revision now requires changes to proceed.Nov 10 2023, 12:56 PM

rohan added inline comments.Nov 10 2023, 1:44 PM

keyserver/src/database/migration-config.js
684	Oh yeah this was a mistake on my part. It the number of ids should probably be the length of the `threads` array below since for each `COMMUNITY_OPEN_ANNOUNCEMENT_SUBTHREAD` and `COMMUNITY_SECRET_ANNOUNCEMENT_SUBTHREAD`, we'll need a new role

Update createIDs call to use threads.length. Tested this with more than rolePermissions.length (3) threads

Harbormaster completed remote builds in B24013: Diff 33089.Nov 10 2023, 2:47 PM

ashoat accepted this revision.Nov 11 2023, 10:59 AM

This revision is now accepted and ready to land.Nov 11 2023, 10:59 AM

ashoat requested changes to this revision.Nov 11 2023, 3:22 PM

ashoat added inline comments.

keyserver/src/database/migration-config.js
672 ↗	(On Diff #33089)	Is it a problem that you're using the permissions blob for `COMMUNITY_OPEN_ANNOUNCEMENT_SUBTHREAD` here, but the query below is also fetching `COMMUNITY_SECRET_ANNOUNCEMENT_SUBTHREAD`?

This revision now requires changes to proceed.Nov 11 2023, 3:22 PM

rohan added inline comments.Nov 11 2023, 3:54 PM

keyserver/src/database/migration-config.js
672 ↗	(On Diff #33089)	No it won’t be an issue since they return the same permission blobs. I thought since this was the case, there wasn’t much of a point calling getRolePermissionBlobs several times, and having it defined once was better