Details

Reviewers

tomek
atul
• abosh
ashoat

Commits

rCOMMa404cfdd6f73: [keyserver] Add a function for fetching the keyserver admins ID

Summary

A function for fetching the keyserver admins ID, similarly to D4958 - assuming there is only one community and it's admin is the keyservers admin. This will be added to ENG-1707 hack.
This function is needed in message-report creator. The function from D4958 will not be used there as it would be much slower. The function from this diff will not be used where the function from D4958 is
used, because it's used on native and web so we cannot call the database there.

Test Plan

Add the following line to the textMessageCreationResponder from keyserver/src/responders/message-responders.js:

console.log(await fetchKeyserverAdminID());

And see that when a user sends a text message (if you have just one admin in your setup) ashoat users's id (256) is logged to console.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

inka created this revision.Sep 7 2022, 3:14 AM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 7 2022, 3:14 AM

inka edited the summary of this revision. (Show Details)Sep 7 2022, 3:15 AM

inka added a child revision: D4941: [keyserver] Add message report creator.Sep 7 2022, 3:18 AM

Harbormaster completed remote builds in B11946: Diff 16397.Sep 7 2022, 3:24 AM

inka requested review of this revision.Sep 7 2022, 3:24 AM

Change the way we decide which threads are community threads, based on threadTypeIsCommunityRoot function

Harbormaster completed remote builds in B12086: Diff 16587.Sep 12 2022, 7:30 AM

tomek requested changes to this revision.Sep 13 2022, 3:00 AM

tomek added inline comments.

keyserver/src/fetchers/user-fetchers.js
274 ↗	(On Diff #16587)	According to https://linear.app/comm/issue/ENG-1707/revert-fix-getting-the-keyserver-admin-info, in our hack we not only check if it is any admin role, but also check if this role has `CHANGE_ROLE` permission. Can we add this check also here?
280 ↗	(On Diff #16587)	We can consider logging a warning if there are more rows.

This revision now requires changes to proceed.Sep 13 2022, 3:00 AM

Log a warning when multiple potential keyserver admins found, check for CHANGE_ROLE permission

Great! Just one last comment inline

keyserver/src/fetchers/user-fetchers.js
274–275 ↗	(On Diff #16619)	Unfortunately, we don't have any automated tool for formatting SQL, but we have a convention that we indent nested statements by 2 spaces and prefer adding `AND` at the end of the line. Additional note: we should use a constant instead of hardcoding `change_role` permission.

This revision now requires changes to proceed.Sep 13 2022, 6:49 AM

Harbormaster completed remote builds in B12111: Diff 16619.Sep 13 2022, 6:50 AM

inka added inline comments.Sep 13 2022, 6:52 AM

keyserver/src/fetchers/user-fetchers.js
282 ↗	(On Diff #16619)	Not sure if it should say "community admin" or "keyserver admin" On one hand the function is for fetching the keyserver admin, but on the other hand what actually happens is we find multiple community admins. Maybe it should say something more informative, like "more than one community admin found, assuming the keyserver admin to be of lowest id"

Make code prettier

keyserver/src/fetchers/user-fetchers.js
287 ↗	(On Diff #16620)	Not sure if it should say "community admin" or "keyserver admin" On one hand the function is for fetching the keyserver admin, but on the other hand what actually happens is we find multiple community admins. Maybe it should say something more informative, like "more than one community admin found, assuming the keyserver admin to be of lowest id" (Repeating so it doesn't get lost)

Harbormaster completed remote builds in B12112: Diff 16620.Sep 13 2022, 7:18 AM

Looks great! Adding @ashoat so that he can take a look - this solution seems to be consistent with our current https://linear.app/comm/issue/ENG-1707/revert-fix-getting-the-keyserver-admin-info.

keyserver/src/fetchers/user-fetchers.js
287 ↗	(On Diff #16620)	I think the current message is fine, but improving it is also a good idea. Also curious about @ashoat opinion. About repeating stuff: there's a setting that allows displaying all the comments, not only the ones from the most recent reviosion.