[CP Staging][Payment due @mkhutornyi] Render agent rule change logs in NewDot #admins - #96773
Conversation
This comment has been minimized.
This comment has been minimized.
|
(Yuwen's Agent) Squawk received 🦜 — adopted the suggested translations for all 9 locales in a104f43. |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@youssef-lr Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
a18533e
This comment has been minimized.
This comment has been minimized.
|
(Yuwen's Agent) Squawk received 🦜 — adopted most of these, but held back a few on closer inspection:
Everything else (de.ts, nl.ts, pl.ts) applied as suggested. |
|
If this requires C+ review, I can help |
|
I believe we don't have need to hold this anymore? @yuwenmemon ? |
|
Yeah, the web PR got deployed to staging, so I think we're probably good to move forward here. |
|
@mkhutornyi great, can you review ASAP? |
mkhutornyi
left a comment
There was a problem hiding this comment.
There are some translation copy bugs
- de.ts —
updatedfallback drops the auxiliary verb:eine Agentenregel aktualisiert zu: …should behat eine Agentenregel aktualisiert zu:. Every other string in the block (and its neighbors) starts withhat. - pl.ts — voice flips between branches within the same key:
dodał(a)vsdodano,usunął(-ę)vsusunięto. Alsousunął(-ę)looks like a typo forusunął(-ęła). - nl.ts — three different quote styles across three adjacent lines:
"${title}",„${title}”,‘${title}’. Neighboring nl strings use‘…’. - es.ts —
la regla de agente(added/deleted) vsla regla del agente(updated). - it.ts —
la regola dell'agentevsuna regola agenteacross branches.
@MelvinBot can you confirm?
|
Confirmed — all five are real. Each is in the strings added by this PR (
(Note: these are per-locale typographic quotes in the content — the |
|
(Yuwen's Agent) All 5 confirmed — fixed in c8da262:
All re-verified via jest + typecheck-tsgo, pushed. |
ReviewSolid, well-scoped PR. All five render paths are wired consistently (main content, LHN last-message, sidebar alternate text, thread report name, and context-menu copy), all 10 locales are covered, A couple of things worth a look before merge: 1. LHN preview isn't concise for add/update — contradicts Test step 5 Test step 5 says the LHN preview should show "the concise 'added/updated/deleted an agent rule' text." But the LHN reuses the same full builder for add/update, so it renders the entire prompt inline:
Only the empty-title delete fallback ( 2. Minor: verify the empty-title fallbacks are actually reachable The PR notes the title is server-generated and "may be empty," and every builder has a prompt-only / generic fallback (nicely tested). Just confirm the companion Auth PR emits Nothing blocking. The typography looks locale-appropriate (e.g. |
|
Please merge main. Typecript check is fixed in #97370 |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Regression scanNo behavioral regression to existing functionality found. Every change is additive and keyed on three brand-new action types ( Things I specifically checked for regressions and cleared:
Two watch items (not regressions, and one is unverified):
|
|
@mkhutornyi thanks! Done! |
9bc1149 to
dcc21c2
Compare
|
🎯 @mkhutornyi, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
🚧 puneetlath has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
…gelog [Payment due @mkhutornyi] Render agent rule change logs in NewDot #admins (cherry picked from commit 5f2041f) (cherry-picked to staging by puneetlath)
|
🚀 Cherry-picked to staging by https://github.com/puneetlath in version: 9.4.46-1 🚀
|
|
🤖 Help site review — docs update required This PR adds a user-visible admin behavior that the help site doesn't currently document: whenever an Agent rule is added, updated, or deleted, a system message is now recorded in the workspace's #admins room as an audit trail. The existing Create Agent Rules article covers creating, writing, and deleting Agent rules, but it doesn't mention that these changes are logged in #admins. I've drafted a doc update that adds a task-based section ( Draft help site PR: #97398 No other help site articles reference the agent-rule change log, so this is the only docs change needed for this PR. @yuwenmemon, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #97416 was identified to be related to this PR. |
@puneetlath please review
Explanation of Change
Renders the three new agent-rule change-log system messages in NewDot's #admins room, so admins get a visible audit trail whenever an agent rule is added, updated, or deleted. Add/update messages show the rule title and full prompt; delete shows just the title (the title is server-generated and may be empty, so each message falls back to a generic phrasing).
The backend that emits these
POLICYCHANGELOG_{ADD,UPDATE,DELETE}_AGENT_RULEactions is the companion Auth PR https://github.com/Expensify/Auth/pull/23134. Deploy this NewDot PR first so the messages render as soon as Auth starts emitting them; an unhandled subtype simply renders nothing (no crash). OldDot's own rendering is the companion Web-Expensify PR https://github.com/Expensify/Web-Expensify/pull/54776.Fixed Issues
For https://github.com/Expensify/Expensify/issues/660979
PROPOSAL:
Tests
(Requires the companion Auth PR on the same environment to emit the actions.)
Offline tests
These are server-emitted system messages, so no new offline behavior is introduced — an already-synced #admins room shows the cached messages while offline.
QA Steps
Same as tests (run on staging once the companion Auth change is deployed there).
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos