Page MenuHomePhabricator
Differential D14510

[lib] Introduce delete message type
AcceptedPublic
Actions

Authored by tomek on Fri, Mar 28, 4:10 AM.
Tags
None
Referenced Files
F5237698: D14510.diff
Sat, Apr 5, 3:31 PM
F5236393: D14510.id47608.diff
Sat, Apr 5, 12:31 PM
Unknown Object (File)
Sat, Apr 5, 11:59 AM
Unknown Object (File)
Fri, Apr 4, 7:36 PM
Unknown Object (File)
Fri, Apr 4, 5:39 PM
Unknown Object (File)
Fri, Apr 4, 1:15 PM
Unknown Object (File)
Fri, Apr 4, 6:25 AM
Unknown Object (File)
Thu, Apr 3, 11:22 PM
View All 22 Files
Subscribers
None

Details

Reviewers
kamil
ashoat
Summary

Introduce a new message type for deleting a message

https://linear.app/comm/issue/ENG-10325/create-a-new-message-type

Depends on D14505

Test Plan

Flow

Diff Detail

Repository
rCOMM Comm
Branch
delete-messages
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek created this revision.Fri, Mar 28, 4:10 AM
Harbormaster completed remote builds in B33861: Diff 47608.Fri, Mar 28, 4:29 AM
tomek requested review of this revision.Fri, Mar 28, 4:29 AM
tomek updated this revision to Diff 47609.Fri, Mar 28, 4:44 AM

Modify some more types, test the validator

tomek added inline comments.Fri, Mar 28, 4:46 AM
lib/selectors/chat-selectors.js
461

This is temporary just to make sure Flow doesn't complain. Implementing the proper solution is a part of the client side work.

lib/shared/messages/delete-message-spec.js
65

We're introducing this whole spec just to have this line - to make sure the tests work. The functions that were introduced here are mandatory. More functions will be implemented later.

Harbormaster completed remote builds in B33862: Diff 47609.Fri, Mar 28, 5:03 AM
tomek added a child revision: D14511: [lib] Introduce delete message spec.Fri, Mar 28, 5:45 AM
ashoat accepted this revision.Fri, Mar 28, 6:42 AM

Can you retitle all of the diffs in this stack to explicitly reference message deletion? Just realized that they're just generically titled.

This revision is now accepted and ready to land.Fri, Mar 28, 6:42 AM
tomek updated this revision to Diff 47611.Fri, Mar 28, 6:46 AM

Expand MessageData

Harbormaster failed remote builds in B33864: Diff 47611!Fri, Mar 28, 6:55 AM
tomek retitled this revision from [lib] Introduce a new message type to [lib] Introduce delete message type.Thu, Apr 3, 8:16 AM
In D14510#402801, @ashoat wrote:

Can you retitle all of the diffs in this stack to explicitly reference message deletion? Just realized that they're just generically titled.

Sure!

Revision Contents
Changeset List

PathSize
lib/
selectors/
3 lines
shared/
10 lines
messages/
66 lines
2 lines
types/
4 lines
12 lines
messages/
39 lines
9 lines
8 lines

Diff 47609

View Options

lib/selectors/chat-selectors.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

lib/shared/message-utils.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

lib/shared/messages/delete-message-spec.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

lib/shared/messages/message-specs.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

lib/types/message-types-enum.js

Loading...
View Options

lib/types/message-types.js

Loading...
View Options

lib/types/messages/delete.js

Loading...
View Options

lib/types/messages/unsupported.js

Loading...
View Options

lib/types/validation.test.js

Loading...