Repository navigation
FFS-4862: Ensure "Reporting for (name)" header appears throughout Emmy CE and Employment flows - #2161
Conversation
| def show_activity_reporting_banner? | ||
| false | ||
| end |
There was a problem hiding this comment.
CE pages will display the banner by default unless you opt out like this.
There was a problem hiding this comment.
nonblocking, dev chat probably: do we stylistically want to favor patterns like this where its (meh) in the controller but (yay) comes configured for free in the flow, or more controlled in the UI which has its own tradeoffs?
There was a problem hiding this comment.
This seemed slightly less clunky to me than the way I originally had it, which was essentially just checking against URLs. It's a good question.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed 15 files against origin/main (security perspective only; no IaC files in the diff). Found 0 findings. The new 'Reporting for ' banner HTML-escapes the applicant name (tag.strong plus Rails' _html translation helper), is limited to the current session's or invitation's ActivityFlow, and is not written to logs, tracking or URLs. Spec data uses synthetic names only.
Reviewed by AI, was this helpful? Please react with 👍 or 👎.
| return unless flow.is_a?(ActivityFlow) && flow.activity_flow_invitation.present? | ||
|
|
||
| applicant = flow.cbv_applicant | ||
| return unless applicant&.first_name.present? && applicant.last_name.present? |
There was a problem hiding this comment.
nit: rather than applicant&.ing, i'd add that as another escape hatch to the control flow. alternatively, would there ever be a reason to assume the cbv_applicant is missing here? wouldnt things get pretty screwy in that case?
There was a problem hiding this comment.
Yeah, that's a good call. This was me being unnecessarily defensive since you're correct that we will have an applicant value here. I'll rework this a bit to match what you're getting at. Thanks!
iannorriswork
left a comment
iannorriswork
left a comment
There was a problem hiding this comment.
left some thinking comments, but nothing blocking. checked and the name is showing up as appropriate in CE flow.
|
test-classifier: AI triage of failing tests AI Test Classifier — no action required
Re-resolved base: AI_REVIEW_DIFF_RANGE (origin/main HEAD) included an upstream commit not yet merged into the branch (a04e99a), so I used merge-base 3abdd37 as the base. Ran the changed specs (165 examples), the changed e2e spec with E2E_RUN_TESTS=1 (3 examples) and the wider controllers/components/helpers/requests suites (1620 examples), all with 0 failures. Used Ruby 3.4.11 (repo pins 3.4.9) with the Gemfile pin restored afterward; built npm assets before the final run. React 👍 if this is right (nothing needed triage) / 👎 if a real failure was missed, and on a 👎 please reply with a one-line reason. Advisory, non-blocking. |
|
Excellent work, Jake! Approved! ✅ |
This branch was successfully deployed
3 participants
FFS-4862
Changes
This PR adds the
Reporting for <name>header across the Emmy CE / employment-focused flows.This header does not display on a few of the CE pages (also shown here):
The header will not display if first name or last name is blank / empty. The header never displays when using the "income" flow type.
Context for reviewers
Example
Acceptance testing
Tag product and design in Slack for acceptance: @emmy-acceptance-testers
:alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!)AI Usage
If you used AI:
Purpose for AI usage:
Which AI Tool(s) did you use:
Infrastructure Changes
Risk / Downtime:
CMS Cloud review environment
♻️ Environment destroyed ♻️