Page MenuHomePhabricator

[keyserver] Update usage of threads.default_role in validate-role-permissions.js
ClosedPublic

Authored by rohan on Nov 24 2023, 9:55 AM.
Tags
None
Referenced Files
F3184654: D9978.diff
Fri, Nov 8, 11:23 AM
Unknown Object (File)
Mon, Nov 4, 6:34 PM
Unknown Object (File)
Oct 2 2024, 4:13 AM
Unknown Object (File)
Oct 2 2024, 4:13 AM
Unknown Object (File)
Oct 2 2024, 4:13 AM
Unknown Object (File)
Oct 2 2024, 4:12 AM
Unknown Object (File)
Oct 2 2024, 4:09 AM
Unknown Object (File)
Aug 28 2024, 11:54 AM
Subscribers

Details

Summary

We need to get rid of usages of threads.default_role throughout the codebase so we can eventually drop the column.

This diff handles the usage in validate-role-permissions.js

Existing code: Fetches the default_role for the thread for each role so we can determine what permissions blob the role should have.

Depends on D9977

Addresses part of ENG-5833

Test Plan

Ran the script before and after the changes and logged the results of the query. I put them into a side by side text comparison on https://text-compare.com/, and confirmed that whenever default_role was equal to the roleID, the new is_default field = 1. Otherwise, it is null (casted to 0 during the Boolean conversion).

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)
keyserver/src/scripts/validate-role-permissions.js
23 ↗(On Diff #33604)

Indent please

tomek requested changes to this revision.Nov 27 2023, 3:06 AM
tomek added inline comments.
keyserver/src/scripts/validate-role-permissions.js
22–23 ↗(On Diff #33604)

Why do we assume that a role is default instead of checking a condition if it is default? (Should r.special_role = ${specialRoles.DEFAULT_ROLE} be a condition in WHERE?)

This revision now requires changes to proceed.Nov 27 2023, 3:06 AM
tomek added inline comments.
keyserver/src/scripts/validate-role-permissions.js
22–23 ↗(On Diff #33604)

Ah, after a second read, it makes sense!

This revision is now accepted and ready to land.Nov 27 2023, 3:12 AM