Page MenuHomePhabricator
Differential D3670

[web] Allow passing `Modal` parameters into `SearchModal`
ClosedPublic
Actions

Authored by • jacek on Apr 8 2022, 5:56 AM.
Tags
None
Referenced Files
F6303004: D3670.id11936.diff
Sat, Apr 26, 6:21 AM
F6303002: D3670.id11468.diff
Sat, Apr 26, 6:21 AM
F6302999: D3670.diff
Sat, Apr 26, 6:21 AM
Unknown Object (File)
Thu, Apr 24, 7:16 AM
Unknown Object (File)
Fri, Apr 18, 2:59 PM
Unknown Object (File)
Fri, Apr 18, 1:32 PM
Unknown Object (File)
Fri, Apr 18, 9:12 AM
Unknown Object (File)
Fri, Apr 18, 1:59 AM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
• benschac
• karol
tomek

Details

Reviewers
tomek
• benschac
atul
Commits
rCOMMd0de89e4792c: [web] Allow passing `Modal` parameters into `SearchModal`
Summary

Allow setting modal properties when using SearchModal. Will be used in following diffs.

Test Plan

All modals should work as before. Can be tested after next diff in stack.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• jacek created this revision.Apr 8 2022, 5:56 AM
Herald added subscribers: • abosh, • benschac, • karol and 4 others. · View Herald TranscriptApr 8 2022, 5:56 AM
• jacek added a child revision: D3671: [web] Adjust `MembersModal` size to match designs.Apr 8 2022, 5:59 AM
• jacek added a child revision: D3672: [web] Adjust `SubchannelsModal` size to match designs.
Harbormaster completed remote builds in B8022: Diff 11227.Apr 8 2022, 6:02 AM
• jacek requested review of this revision.Apr 8 2022, 6:02 AM
• jacek added a reviewer: tomek.Apr 8 2022, 6:07 AM
tomek accepted this revision.Apr 11 2022, 6:50 AM

I've opened a similar diff D3644, but feel free to land yours

This revision is now accepted and ready to land.Apr 11 2022, 6:50 AM
• jacek updated this revision to Diff 11468.Apr 14 2022, 3:15 AM

Allow passing other Modal parameters

• jacek requested review of this revision.Apr 14 2022, 3:16 AM
• jacek retitled this revision from [web] Allow passing `size` parameter into `SearchModal` to [web] Allow passing `Modal` parameters into `SearchModal`.
• jacek edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B8197: Diff 11468.Apr 14 2022, 3:22 AM
• jacek added reviewers: • benschac, atul.Apr 15 2022, 3:07 AM
• jacek added a child revision: D3739: [web] Introduce `AddMembersItem` component.Apr 15 2022, 4:04 AM
tomek requested changes to this revision.Apr 15 2022, 8:41 AM

Looks really nice!

web/modals/modal.react.js
10–16

I think we should split this type into two parts: with overridable and non-overridable props. The second category contains only children. Currently we replace it with +children: (searchText: string) => React.Node, but it would be cleaner to not include it in ModalProps - it will make this code more maintainable.

18

We usually try not to mix default and named exports. This component already had this issue, but we can easily fix that

This revision now requires changes to proceed.Apr 15 2022, 8:41 AM
ashoat added inline comments.Apr 18 2022, 12:24 PM
web/modals/modal.react.js
18
  1. Please avoid inline export statements, except in files like lib/types that have a ton of exports. Instead, have a single export line at the bottom
  2. We have a lot of places where we export default with no export, but do we have export type. I don't feel strongly, but worth noting that today the standard in the codebase about export default vs. export does not apply to export type
• jacek updated this revision to Diff 11630.Apr 19 2022, 10:59 AM

Excluded overridable Modal props into separate type

Harbormaster completed remote builds in B8325: Diff 11630.Apr 19 2022, 11:38 AM
tomek accepted this revision.Apr 20 2022, 9:40 AM
This revision is now accepted and ready to land.Apr 20 2022, 9:40 AM
• jacek updated this revision to Diff 11880.Apr 25 2022, 10:28 AM

rebase

Harbormaster completed remote builds in B8521: Diff 11880.Apr 25 2022, 10:42 AM
• jacek updated this revision to Diff 11925.Apr 26 2022, 8:34 AM

rebase

Harbormaster completed remote builds in B8556: Diff 11925.Apr 26 2022, 8:49 AM
Closed by commit rCOMMd0de89e4792c: [web] Allow passing `Modal` parameters into `SearchModal` (authored by • jacek). · Explain WhyApr 26 2022, 9:09 AM
This revision was automatically updated to reflect the committed changes.
• jacek added a commit: rCOMMd0de89e4792c: [web] Allow passing `Modal` parameters into `SearchModal`.

Revision Contents
Changeset List

PathSize
web/
modals/
4 lines
11 lines

Diff 11468

View Options

web/modals/modal.react.js

Show All 21 Lines
import { dbQuery, SQL } from '../database/database.js'; import { dbQuery, SQL } from '../database/database.js';
import { import {
fetchThreadInfos, fetchThreadInfos,
fetchServerThreadInfos, fetchServerThreadInfos,
rawThreadInfosFromServerThreadInfos, rawThreadInfosFromServerThreadInfos,
} from '../fetchers/thread-fetchers.js'; } from '../fetchers/thread-fetchers.js';
import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js'; import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';
import type { Viewer } from '../session/viewer.js'; import type { Viewer } from '../session/viewer.js';
import { updateRole } from '../updaters/thread-updaters.js';
type InitialRoles = { type InitialRoles = {
+default: RoleInfo, +default: RoleInfo,
+creator: RoleInfo, +creator: RoleInfo,
}; };
async function createInitialRolesForNewThread( async function createInitialRolesForNewThread(
threadID: string, threadID: string,
threadType: ThreadType, threadType: ThreadType,
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines const hasPermission = await checkThreadPermission(
viewer, viewer,
request.community, request.community,
threadPermissions.CHANGE_ROLE, threadPermissions.CHANGE_ROLE,
); );
if (!hasPermission) { if (!hasPermission) {
throw new ServerError('invalid_credentials'); throw new ServerError('invalid_credentials');
} }
const { community, name, permissions, action } = request; const { community, existingRoleID, name, permissions, action } = request;
for (const permission of permissions) { for (const permission of permissions) {
if (!userSurfacedPermissionsSet.has(permission)) { if (!userSurfacedPermissionsSet.has(permission)) {
throw new ServerError('invalid_parameters'); throw new ServerError('invalid_parameters');
} }
} }
const [id] = await createIDs('roles', 1); const [id] = await createIDs('roles', 1);
Show All 31 Lines): Promise<RoleModificationResult> {
let query = SQL``; let query = SQL``;
if (action === 'create_role') { if (action === 'create_role') {
query = SQL` query = SQL`
INSERT INTO roles (id, thread, name, permissions, creation_time) INSERT INTO roles (id, thread, name, permissions, creation_time)
VALUES (${row}) VALUES (${row})
`; `;
} else if (action === 'edit_role') { } else if (action === 'edit_role') {
throw new ServerError("unimplemented: can't edit roles yet"); query = SQL`
UPDATE roles
SET name = ${name}, permissions = ${permissionsBlob}
WHERE id = ${existingRoleID}
`;
} }
await dbQuery(query); await dbQuery(query);
// The `updateRole` needs to occur after the role has been updated
// in the database because it will want the most up to date role info
// (permissions / name)
if (action === 'edit_role') {
atulUnsubmitted
Not Done
Inline Actions

Instead of two separate (action === 'edit_role') blocks, could we merge the two?

We could move await dbQuery(query); within both the (action === 'create_role') (line 139) and else if (action === 'edit_role') (line 144) blocks?

Not a huge deal, whatever you prefer

atul: Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah we can, that should be cleaner too

rohan: Yeah we can, that should be cleaner too
if (!existingRoleID) {
throw new ServerError('invalid_parameters');
}
const membersWithRole = threadInfo.members.filter(
memberInfo => memberInfo.role === existingRoleID,
);
await updateRole(
viewer,
{
threadID: community,
role: existingRoleID,
memberIDs: membersWithRole.map(memberInfo => memberInfo.id),
atulUnsubmitted
Not Done
Inline Actions

Should we add this .map to the end of the .filter on lines 162-164?

atul: Should we add this `.map` to the end of the `.filter` on lines 162-164?
rohanAuthorUnsubmitted
Done
Inline Actions

Sure that makes sense

rohan: Sure that makes sense
},
{ silenceNewMessages: true },
);
}
const fetchServerThreadInfosResult = await fetchServerThreadInfos({ const fetchServerThreadInfosResult = await fetchServerThreadInfos({
threadID: community, threadID: community,
}); });
const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult; const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;
const serverThreadInfo = serverThreadInfos[community]; const serverThreadInfo = serverThreadInfos[community];
if (!serverThreadInfo) { if (!serverThreadInfo) {
throw new ServerError('internal_error'); throw new ServerError('internal_error');
Show All 32 Lines
View Options

web/modals/search-modal.react.js

Show All 21 Lines
import { dbQuery, SQL } from '../database/database.js'; import { dbQuery, SQL } from '../database/database.js';
import { import {
fetchThreadInfos, fetchThreadInfos,
fetchServerThreadInfos, fetchServerThreadInfos,
rawThreadInfosFromServerThreadInfos, rawThreadInfosFromServerThreadInfos,
} from '../fetchers/thread-fetchers.js'; } from '../fetchers/thread-fetchers.js';
import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js'; import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';
import type { Viewer } from '../session/viewer.js'; import type { Viewer } from '../session/viewer.js';
import { updateRole } from '../updaters/thread-updaters.js';
type InitialRoles = { type InitialRoles = {
+default: RoleInfo, +default: RoleInfo,
+creator: RoleInfo, +creator: RoleInfo,
}; };
async function createInitialRolesForNewThread( async function createInitialRolesForNewThread(
threadID: string, threadID: string,
threadType: ThreadType, threadType: ThreadType,
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines const hasPermission = await checkThreadPermission(
viewer, viewer,
request.community, request.community,
threadPermissions.CHANGE_ROLE, threadPermissions.CHANGE_ROLE,
); );
if (!hasPermission) { if (!hasPermission) {
throw new ServerError('invalid_credentials'); throw new ServerError('invalid_credentials');
} }
const { community, name, permissions, action } = request; const { community, existingRoleID, name, permissions, action } = request;
for (const permission of permissions) { for (const permission of permissions) {
if (!userSurfacedPermissionsSet.has(permission)) { if (!userSurfacedPermissionsSet.has(permission)) {
throw new ServerError('invalid_parameters'); throw new ServerError('invalid_parameters');
} }
} }
const [id] = await createIDs('roles', 1); const [id] = await createIDs('roles', 1);
Show All 31 Lines): Promise<RoleModificationResult> {
let query = SQL``; let query = SQL``;
if (action === 'create_role') { if (action === 'create_role') {
query = SQL` query = SQL`
INSERT INTO roles (id, thread, name, permissions, creation_time) INSERT INTO roles (id, thread, name, permissions, creation_time)
VALUES (${row}) VALUES (${row})
`; `;
} else if (action === 'edit_role') { } else if (action === 'edit_role') {
throw new ServerError("unimplemented: can't edit roles yet"); query = SQL`
UPDATE roles
SET name = ${name}, permissions = ${permissionsBlob}
WHERE id = ${existingRoleID}
`;
} }
await dbQuery(query); await dbQuery(query);
// The `updateRole` needs to occur after the role has been updated
// in the database because it will want the most up to date role info
// (permissions / name)
if (action === 'edit_role') {
atulUnsubmitted
Not Done
Inline Actions

Instead of two separate (action === 'edit_role') blocks, could we merge the two?

We could move await dbQuery(query); within both the (action === 'create_role') (line 139) and else if (action === 'edit_role') (line 144) blocks?

Not a huge deal, whatever you prefer

atul: Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could…
rohanAuthorUnsubmitted
Done
Inline Actions

Yeah we can, that should be cleaner too

rohan: Yeah we can, that should be cleaner too
if (!existingRoleID) {
throw new ServerError('invalid_parameters');
}
const membersWithRole = threadInfo.members.filter(
memberInfo => memberInfo.role === existingRoleID,
);
await updateRole(
viewer,
{
threadID: community,
role: existingRoleID,
memberIDs: membersWithRole.map(memberInfo => memberInfo.id),
atulUnsubmitted
Not Done
Inline Actions

Should we add this .map to the end of the .filter on lines 162-164?

atul: Should we add this `.map` to the end of the `.filter` on lines 162-164?
rohanAuthorUnsubmitted
Done
Inline Actions

Sure that makes sense

rohan: Sure that makes sense
},
{ silenceNewMessages: true },
);
}
const fetchServerThreadInfosResult = await fetchServerThreadInfos({ const fetchServerThreadInfosResult = await fetchServerThreadInfos({
threadID: community, threadID: community,
}); });
const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult; const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;
const serverThreadInfo = serverThreadInfos[community]; const serverThreadInfo = serverThreadInfos[community];
if (!serverThreadInfo) { if (!serverThreadInfo) {
throw new ServerError('internal_error'); throw new ServerError('internal_error');
Show All 32 Lines