Page MenuHomePhabricator
Differential D6575

[lib] Render EntityText directly from notifRobotextForMessageInfo
ClosedPublic
Actions

Authored by ashoat on Feb 4 2023, 10:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 1:06 PM
Unknown Object (File)
Sat, Oct 26, 9:22 PM
Unknown Object (File)
Sat, Oct 26, 9:22 PM
Unknown Object (File)
Sat, Oct 26, 9:22 PM
Unknown Object (File)
Sat, Oct 26, 9:22 PM
Unknown Object (File)
Sat, Oct 26, 9:22 PM
Unknown Object (File)
Sat, Oct 26, 9:21 PM
Unknown Object (File)
Sat, Oct 26, 9:14 PM
View All Files
Subscribers
tomek

Details

Reviewers
atul
ginsu
michal
marcin
Commits
rCOMMd0cd85eac1d5: [lib] Render EntityText directly from notifRobotextForMessageInfo
Summary

This diff does three things:

  1. Renames strippedRobotextForMessageInfo to notifRobotextForMessageInfo to better reflect its intended use
  2. Modifies it to return an EntityText directly instead of using entityTextToRawString, which isn't capable of resolving ENS names
  3. Stops passing notifRobotextForMessageInfo from notif-utils into the message specs, and instead just import it from the message specs. Note that this introduces a require cycle, but I think this is fine in modern Node.js (where this code is getting executed)

Depends on D6574

Test Plan

Tested in combination with later notif diffs

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat created this revision.Feb 4 2023, 10:01 AM
Herald added a subscriber: tomek. · View Herald TranscriptFeb 4 2023, 10:01 AM
ashoat added a child revision: D6576: [lib] Migrate CreateEntry/EditEntry notifTexts to use EntityText.Feb 4 2023, 10:02 AM
Harbormaster completed remote builds in B16089: Diff 21976.Feb 4 2023, 1:22 PM
ashoat requested review of this revision.Feb 4 2023, 1:22 PM
atul accepted this revision.Feb 4 2023, 2:22 PM
This revision is now accepted and ready to land.Feb 4 2023, 2:22 PM
ashoat updated this revision to Diff 22034.Feb 5 2023, 7:33 AM

ChangeRole should say "admins of X" not "admins from X". This update changes behavior, but for the better

ashoat updated this revision to Diff 22035.Feb 5 2023, 7:39 AM

Update to handle possessive correctly in ChangeSettings notif.

I noticed that ChangeSettings notifs weren't handled the possessive on the thread correctly. This was working correctly before D6513, where I introduced a regression.

In the past, the entity part of the text didn't handle the possessive. As a result, simply replacing the thread part would leave the possessive in.

Now, the entity part handles the possessive. If we simply replace the entity with the thread name, we drop the possessive.

This diff makes us replace with an entity that preserves the possessive property.

Harbormaster failed remote builds in B16143: Diff 22035!Feb 5 2023, 7:43 AM
Harbormaster completed remote builds in B16142: Diff 22034.Feb 5 2023, 7:48 AM
ashoat updated this revision to Diff 22054.Feb 5 2023, 7:56 AM

Rebase

This revision was landed with ongoing or failed builds.Feb 5 2023, 8:01 AM
Closed by commit rCOMMd0cd85eac1d5: [lib] Render EntityText directly from notifRobotextForMessageInfo (authored by ashoat). · Explain Why
This revision was automatically updated to reflect the committed changes.
ashoat added a commit: rCOMMd0cd85eac1d5: [lib] Render EntityText directly from notifRobotextForMessageInfo.
Harbormaster completed remote builds in B16145: Diff 22054.Feb 5 2023, 8:11 AM

Revision Contents
Changeset List

Diff 22034

View Options

lib/shared/messages/add-members-message-spec.js

Loading...
View Options

lib/shared/messages/change-role-message-spec.js

Loading...
View Options

lib/shared/messages/change-settings-message-spec.js

Loading...
View Options

lib/shared/messages/message-spec.js

Loading...
View Options

lib/shared/messages/remove-members-message-spec.js

Loading...
View Options

lib/shared/notif-utils.js

Loading...