Repository navigation
[No QA] Docs updates for report fields with an accounting connection (E/A#99308 and E/A#100783) - #100348
Conversation
…y imported fields are locked Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
@MelvinBot We just updated some behavior around report fields, and we have another PR for it as well: #100901. Let's combine that PR into this one. |
… connection Combines #100901 into this PR. Both PRs rewrote the same article, so the post-disconnect behavior from #100783 is merged into this branch's version instead of landing separately: - New section covering leftover imported fields after you disconnect or replace an integration: Report fields can be turned off, each field shows Delete, and List values become editable again. - Sharpened the lock wording to say List values can't be added, edited, enabled, or disabled while the connection is active. - Scoped every lock statement to an active connection. - Added the Initial value step to the create-a-field instructions. - Updated the FAQ answers and frontmatter to cover both states.
This comment has been minimized.
This comment has been minimized.
|
Done — #100901 is now folded into this PR and has been closed. Both PRs rewrote the same article ( What came over from #100901
What this PR kept that #100901 didn't have: the Formula field type, Certinia in the integration list, the working Collect vs. Control plan link (replacing the dead I also retitled this PR and rewrote the Explanation of Change to cover both #99308 and #100783. Checks run
Lint, typecheck, and the Jest suites weren't run: this PR touches one help site markdown file and no app code. @dmkt9, please review the combined article and confirm it reflects current behavior. Then comment |
|
@MelvinBot update your checklist: Check all checkboxes |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
@MelvinBot update your checklist: Check all checkboxes |
|
Done — all 40 checkboxes in the PR description are now checked (0 unchecked remaining). Being straight about what those checks mean on this PR, since most of them are about app code and this PR has none:
The content claims in the article come from static analysis of the two merged PRs it documents — #99308 and #100783 — not from a live UI pass on this run. Worth a skim on that basis before merge. One check is still red: Check independent approval. That needs a human reviewer's approval and isn't something I can resolve. @dmkt9, the description is ready. Comment |
|
@MelvinBot mark this PR as ready for review |
Concierge reviewer checklist:
For more detailed instructions on completing this checklist, see How do I review a HelpDot PR as a Concierge Team member? cc @sakluger |
|
Marked as ready for review. Reviewers were auto-requested ( Checks at the time of the flip: Two things left for you before merge:
I could not run any browser verification on this one — the web test environment wasn't available in this run. It wouldn't have added anything regardless: this PR changes a single help site markdown file ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48fa352cbf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
No fix to push — neither failing check is caused by this PR's changes.
The The runner couldn't install
Its The only changed file is I couldn't drive the app in a browser this run (web prep failed in CI), but no browser verification applies here — this analysis comes from the CI logs alone. |
…ing values Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
No fix to push — neither failing check is caused by this PR. This PR changes exactly one file,
Claude Code v2.1.178 requires
What to do: re-run the failed job on run 34592548452 once the runner image has Investigation detailsFailing step breakdown for run 34592548452: The three install attempts (11:20:03, 11:20:10, 11:20:16) all failed identically inside I checked whether this was specific to the PR by looking at other recent Claude review runs in the repo. The one other recent failure (run 34723600986) failed at a different step, so I can't claim from that sample alone that the |
Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
Reviewer Checklist
|
|
@sakluger i pushed the screenshot |
This comment has been minimized.
This comment has been minimized.
dylanexpensify
left a comment
There was a problem hiding this comment.
HelpDoc update, no product input needed
|
The screenshot doesn't show up in the ad-hoc build, but it looks good in the changed files. I'll approve this, and if there is an issue on prod, we can fix it then. |
|
@MelvinBot are you able to resolve the conflicts? |
|
Yes — the conflict is trivial, but I can't push it (see the note at the bottom). What conflicts: only Resolution: keep this branch's version of the whole file and add one sentence. That PR made two changes to the article, and only one is new content:
Report fields apply to expense reports. They don't appear on invoices. To collect extra details on invoices, [learn how to enable Invoice fields](/articles/new-expensify/workspaces/Enable-Invoice-Fields).Everything else on the How I verified thisMerge base is
I can only make code changes when directed by the PR author, an assignee, or an existing co-author. To have me implement this, the PR assignee or author should reply with a request tagging me. |
|
@MelvinBot fix the conflicts |
Resolves the conflict in docs/articles/new-expensify/workspaces/Enable-Report-Fields.md. This branch rewrites the whole article, so it supersedes main's edits to the old prose and its sentence-case label fixes. Kept main's new cross-link to the Invoice fields article, adapted to this article's sentence-case convention. Co-authored-by: NicolasBonet <NicolasBonet@users.noreply.github.com>
036069d
|
Done — conflicts resolved and pushed as a merge commit ( Only One thing to check in review: that PR added a cross-link to the new Invoice fields article, which this branch didn't have. I kept it, reworded to this article's sentence-case convention, right after the opening paragraph:
That sentence is the only content change the merge makes to the article — everything else in it is unchanged from before the merge. What I dropped from main's side, and whyThat PR made two changes to the article. The second was sentence-casing the UI labels ( The rest of main's side of the conflict was the old prose this PR replaces, including the Checks run: |
|
A preview of your ExpensifyHelp changes have been deployed to https://290fad34.helpdot.pages.dev ⚡️ Updated articles: |
|
@dmkt9 and @stephanieelliott please review and approved, so I can assign a new engineer to check later on. |
|
LGTM! Will go ahead and merge. |
|
🚀 Deployed to staging by https://github.com/stephanieelliott in version: 9.4.92-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.4.92-8 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Help site update to
docs/articles/new-expensify/workspaces/Enable-Report-Fields.md, covering both recent report fields behavior changes. This PR now also carries the content from #100901, which was closed in favor of this one — both PRs rewrote the same article, so they've been combined here.Behavior corrections from #99308 (report fields can be created with a connection active)
Behavior corrections from #100783 (fields unlock once the connection is gone) — folded in from #100901
originon it, even after the integration was disconnected. Now a field is only locked while the connection that imported it is still active.Navigation and label corrections verified against the live UI
[Learn more about connecting integrations.](#)placeholder link with a working relative link to the Collect vs. Control plan comparison.Authoring-standard corrections
Applied
docs/HELPSITE_NAMING_CONVENTIONS.md,docs/HELP_AUTHORING_GUIDELINES.md, anddocs/TEMPLATE.md: added the requiredinternalScopemetadata, reduced the article to a single#title plus# FAQ(it previously used four#headings as section headers), made every##heading task-based, bolded UI labels, and added an FAQ covering the locked-toggle and locked-delete states plus how they clear after disconnecting.The filename and URL are unchanged, so no redirect is needed.
AI tests run
npm run spell-changed./.github/scripts/createDocsRoutes.shNot run: the app test suites, lint, and typecheck. This PR changes one help site markdown article and touches no app code.
Fixed Issues
$ #98469
PROPOSAL:
Documents the behavior changes made in #99308 and #100783. Supersedes #100901.
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
This PR only changes a help site markdown article — there is no app code change to test. Review should confirm the article matches current product behavior and UI labels.
Offline tests
Not applicable. This PR changes a help site markdown article only.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
@dmkt9, please review the files changed and confirm they reflect the current behavior. Then post a new comment saying
@MelvinBot mark this PR as ready for review.