[lib] Fix resetting thick thread avatars
ClosedPublic
Actions

Authored by bartek on Sep 11 2024, 1:29 AM.

Details

Reviewers

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.

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

bartek held this revision as a draft.

bartek published this revision for review.Sep 11 2024, 1:55 AM

bartek added reviewers: tomek, inka.

bartek added inline comments.

lib/shared/dm-ops/change-thread-settings-spec.js
69 ↗	(On Diff #44025)	Empty string is the value that causes avatar to reset - keyserver does it the same way here
lib/types/dm-ops.js
291 ↗	(On Diff #44025)	Wondering if instead of `null`, it's better to use UpdateUserAvatarRemoveRequest here. This would make the above ternary much cleaner

tomek added inline comments.

lib/shared/dm-ops/change-thread-settings-spec.js
67–72 ↗	(On Diff #44025)	Can we simplify this expression and avoid nested ternaries?
lib/types/dm-ops.js
291 ↗	(On Diff #44025)	That makes sense but maybe we should use `UpdateUserAvatarRequest` then

This revision is now accepted and ready to land.Sep 11 2024, 2:07 AM

lib/shared/dm-ops/change-thread-settings-spec.js
67–72 ↗	(On Diff #44025)	Yeah sure
lib/types/dm-ops.js
291 ↗	(On Diff #44025)	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`