Page MenuHomePhabricator
Differential D13283

[lib] Fix resetting thick thread avatars
ClosedPublic
Actions

Authored by bartek on Wed, Sep 11, 1:29 AM.
Tags
None
Referenced Files
F2765676: D13283.id44165.diff
Thu, Sep 19, 2:58 PM
F2764814: D13283.id44144.diff
Thu, Sep 19, 1:28 PM
F2764813: D13283.id44027.diff
Thu, Sep 19, 1:28 PM
F2761168: D13283.id.diff
Thu, Sep 19, 7:28 AM
F2756284: D13283.id.diff
Thu, Sep 19, 12:18 AM
F2755815: D13283.id44164.diff
Wed, Sep 18, 10:27 PM
Unknown Object (File)
Wed, Sep 18, 5:51 PM
Unknown Object (File)
Wed, Sep 18, 1:43 PM
View All 23 Files
Subscribers
ashoat
tomek

Details

Reviewers
kamil
tomek
inka
Commits
rCOMMe0b1a8417c08: [lib] Fix resetting thick thread avatars
Summary

Addresses ENG-9208.
The case when avatar was reset for DM thread, wasn't supported in the thread-actions.js and later in the DM op.

Investigated how this is handled on keyserver and added appropriate logic here.

Test Plan

Confirmed that clicking the "Reset avatar" button in thread settings menu actually resets the avatar.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek created this revision.Wed, Sep 11, 1:29 AM
bartek held this revision as a draft.
Herald added subscribers: tomek, ashoat. · View Herald TranscriptWed, Sep 11, 1:29 AM
bartek added a child revision: D13285: [lib] Support image avatars in DM thread-action.Wed, Sep 11, 1:36 AM
Harbormaster completed remote builds in B31521: Diff 44025.Wed, Sep 11, 1:46 AM
bartek published this revision for review.Wed, Sep 11, 1:55 AM
bartek added reviewers: tomek, inka.
bartek added inline comments.
lib/shared/dm-ops/change-thread-settings-spec.js
69

Empty string is the value that causes avatar to reset - keyserver does it the same way here

lib/types/dm-ops.js
291

Wondering if instead of null, it's better to use UpdateUserAvatarRemoveRequest here. This would make the above ternary much cleaner

tomek accepted this revision.Wed, Sep 11, 2:07 AM
tomek added inline comments.
lib/shared/dm-ops/change-thread-settings-spec.js
67–72

Can we simplify this expression and avoid nested ternaries?

lib/types/dm-ops.js
291

That makes sense but maybe we should use UpdateUserAvatarRequest then

This revision is now accepted and ready to land.Wed, Sep 11, 2:07 AM
bartek added inline comments.Wed, Sep 11, 2:16 AM
lib/shared/dm-ops/change-thread-settings-spec.js
67–72

Yeah sure

lib/types/dm-ops.js
291

I guess it's not a good idea because we still need to convert UpdateUserAvatarRequest -> ClientAvatar at some point. Currently, we do it earlier, in thread-actions.js

bartek added inline comments.Fri, Sep 13, 4:10 AM
lib/types/dm-ops.js
291

After refactoring ternaries I think null is clear enough, also left a comment

bartek updated this revision to Diff 44142.Fri, Sep 13, 4:10 AM

Replace ternaries to make conditions more readable

Harbormaster completed remote builds in B31597: Diff 44142.Fri, Sep 13, 4:30 AM
Closed by commit rCOMMe0b1a8417c08: [lib] Fix resetting thick thread avatars (authored by bartek). · Explain WhyFri, Sep 13, 5:45 AM
This revision was automatically updated to reflect the committed changes.
bartek added a commit: rCOMMe0b1a8417c08: [lib] Fix resetting thick thread avatars.

Revision Contents
Changeset List

PathSize
lib/
actions/
5 lines
shared/
dm-ops/
11 lines
types/
2 lines

Diff 44025

View Options

lib/actions/thread-actions.js

Loading...
View Options

lib/shared/dm-ops/change-thread-settings-spec.js

Loading...
View Options

lib/types/dm-ops.js

Loading...