Page MenuHomePhabricator
Differential D13316

[lib] DMOperationSpec for create entry operation
ClosedPublic
Actions

Authored by will on Sep 12 2024, 10:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 12, 8:28 PM
Unknown Object (File)
Tue, Mar 11, 7:54 PM
Unknown Object (File)
Tue, Mar 11, 9:46 AM
Unknown Object (File)
Wed, Mar 5, 8:16 AM
Unknown Object (File)
Tue, Mar 4, 1:37 PM
Unknown Object (File)
Fri, Feb 28, 5:46 AM
Unknown Object (File)
Fri, Feb 28, 5:46 AM
Unknown Object (File)
Fri, Feb 28, 5:46 AM
View All 84 Files
Subscribers
None

Details

Reviewers
marcin
tomek
ashoat
Commits
rCOMM9d065f191067: [lib] DMOperationSpec for create entry operation
Summary

Create the spec for create entyr operation and getting initial feedback

Depends on D13315

Test Plan

Flow. Testing later in stack

Diff Detail

Repository
rCOMM Comm
Branch
thick-threaded-entries
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

will created this revision.Sep 12 2024, 10:33 PM
Harbormaster completed remote builds in B31583: Diff 44116.Sep 12 2024, 10:50 PM
will requested review of this revision.Sep 12 2024, 10:50 PM
tomek accepted this revision.Sep 13 2024, 2:51 AM
tomek added inline comments.
lib/shared/dm-ops/create-entry-spec.js
75–77

We need to check that the thread exists.

lib/types/dm-ops.js
354–356

It is a bit confusing to see time next to a date because it feels like they represent two parts of the same timestamp. I think we should make it explicit that the date relates to an entry while time to the operation.

This revision is now accepted and ready to land.Sep 13 2024, 2:51 AM
marcin added inline comments.Sep 13 2024, 5:21 AM
lib/shared/dm-ops/create-entry-spec.js
28

Could you please amend the test plan to check for notifs? In order to test notifs simply flip NEXT_CODE_VERSION to 0 and notifs shoudl work automatically.

75

You use methodName() {} syntext here while you use methodName: () => {} syntax for other methods. Could you please, use consistent approach for all methods in this spec? Ideally you would copy the approach that is used in other specs.

ashoat added inline comments.Sep 13 2024, 1:53 PM
lib/shared/dm-ops/create-entry-spec.js
75

Our general approach on picking between these is usually not about consistency in a single file. We opt for methodName() {} when we don't need methodName: () => {} because it's slightly better for performance. We use methodName: () => {} when we want to pass this.methodName somewhere without losing the bound this variable

will updated this revision to Diff 44193.Sep 16 2024, 1:15 AM

rebase

will added a child revision: D13340: [lib] DMOperationSpec for delete entry operation.Sep 16 2024, 1:17 AM
Harbormaster completed remote builds in B31621: Diff 44193.Sep 16 2024, 1:33 AM
Closed by commit rCOMM9d065f191067: [lib] DMOperationSpec for create entry operation (authored by will). · Explain WhySep 16 2024, 2:27 PM
This revision was automatically updated to reflect the committed changes.
will added a commit: rCOMM9d065f191067: [lib] DMOperationSpec for create entry operation.
will marked an inline comment as done.Sep 16 2024, 2:32 PM
will added inline comments.
lib/shared/dm-ops/create-entry-spec.js
75–77

Addressed in rebase

lib/types/dm-ops.js
354–356

Addressed in rebase

will marked an inline comment as done and an inline comment as not done.Sep 16 2024, 3:56 PM
will added inline comments.Sep 16 2024, 3:59 PM
lib/types/dm-ops.js
354–356

Actually, this didn't. Creating a follow up for this: https://linear.app/comm/issue/ENG-9307/rename-create-entry-spec-date-field-to-entrydate-to-avoid-confusing

will added inline comments.Sep 16 2024, 4:01 PM
lib/shared/dm-ops/create-entry-spec.js
28

Addressing in https://linear.app/comm/issue/ENG-9304/verify-notifications-are-working-properly-for-e2ee-entries

Revision Contents
Changeset List

PathSize
lib/
shared/
dm-ops/
81 lines
types/
28 lines

Diff 44116

View Options

lib/shared/dm-ops/create-entry-spec.js

Show First 20 Lines • Show All 367 Lines • ▼ Show 20 Lines) {
return threadMembersWithoutAddedAdmin(threadInfo).length > 2; return threadMembersWithoutAddedAdmin(threadInfo).length > 2;
} }
function threadIsPending(threadID: ?string): boolean { function threadIsPending(threadID: ?string): boolean {
return !!threadID?.startsWith('pending'); return !!threadID?.startsWith('pending');
} }
function threadIsPendingSidebar(threadID: ?string): boolean { function threadIsPendingSidebar(threadID: ?string): boolean {
return !!threadID?.startsWith('pending/sidebar/'); return (
!!threadID?.startsWith('pending/sidebar/') ||
!!threadID?.startsWith('pending/dm_sidebar')
tomekUnsubmitted
Not Done
Inline Actions

Can we extract sidebar and dm_sidebar somewhere as constants?

tomek: Can we extract `sidebar` and `dm_sidebar` somewhere as constants?
);
} }
function getSingleOtherUser( function getSingleOtherUser(
threadInfo: LegacyRawThreadInfo | RawThreadInfo | ThreadInfo, threadInfo: LegacyRawThreadInfo | RawThreadInfo | ThreadInfo,
viewerID: ?string, viewerID: ?string,
): ?string { ): ?string {
if (!viewerID) { if (!viewerID) {
return undefined; return undefined;
▲ Show 20 Lines • Show All 1,407 Lines • Show Last 20 Lines
View Options

lib/types/dm-ops.js

Show First 20 Lines • Show All 367 Lines • ▼ Show 20 Lines) {
return threadMembersWithoutAddedAdmin(threadInfo).length > 2; return threadMembersWithoutAddedAdmin(threadInfo).length > 2;
} }
function threadIsPending(threadID: ?string): boolean { function threadIsPending(threadID: ?string): boolean {
return !!threadID?.startsWith('pending'); return !!threadID?.startsWith('pending');
} }
function threadIsPendingSidebar(threadID: ?string): boolean { function threadIsPendingSidebar(threadID: ?string): boolean {
return !!threadID?.startsWith('pending/sidebar/'); return (
!!threadID?.startsWith('pending/sidebar/') ||
!!threadID?.startsWith('pending/dm_sidebar')
tomekUnsubmitted
Not Done
Inline Actions

Can we extract sidebar and dm_sidebar somewhere as constants?

tomek: Can we extract `sidebar` and `dm_sidebar` somewhere as constants?
);
} }
function getSingleOtherUser( function getSingleOtherUser(
threadInfo: LegacyRawThreadInfo | RawThreadInfo | ThreadInfo, threadInfo: LegacyRawThreadInfo | RawThreadInfo | ThreadInfo,
viewerID: ?string, viewerID: ?string,
): ?string { ): ?string {
if (!viewerID) { if (!viewerID) {
return undefined; return undefined;
▲ Show 20 Lines • Show All 1,407 Lines • Show Last 20 Lines