Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Other income additional comments prefix for page 2 13614c #5620

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

embarnard
Copy link
Contributor

@embarnard embarnard commented Feb 21, 2025

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

  • Yes - don't merge until JIRA issue is accepted!

Reminder: merge main into this branch and get green tests before merging to main

What was done?

  • Added prefix to other income types field in the open text form field and also the pdf
  • opted to add the prefix with javascript and remove it with javascript before form submission and add when creating the pdf because wanted to avoid it being inserted multiple times if the form was loaded multiple times

How to test? Heroku env

  1. Add text to the additional comments field on the second page of the 13614c form in the hub for a client
  2. save the form a few times to test for duplicates
  3. go to the client/documents tab and view the 13614c pdf

Screenshots (for visual changes)

Screenshot 2025-02-24 at 12 33 06 PM Screenshot 2025-02-24 at 12 33 24 PM

Copy link

Heroku app: https://gyr-review-app-5620-8caad679cdd4.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5620 (optionally add --tail)

@@ -29,7 +29,7 @@ def selected_orgs_and_sites
if model.instance_of? Site
model = available_by_id[model.parent_organization_id]
end
model.coalition_id == selected_model.id
model&.coalition_id == selected_model.id
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to this ticket but noticed this will cause this page to crash sometimes; model is nil when can't find model on available_by_id[model.parent_organization_id]

}
});
});
</script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we have a label_and_field method which allows a prefix in honeycrisp that is used by cfa_textarea although cfa_textarea does not pass through a prefix option -- if that was an available option, would that have been a good solution to creating a prefix? Could we create a vita_min_textarea and then create a ticket to update honeycrisp to allow adding a prefix?

@@ -391,7 +391,7 @@

expect(intake.cv_other_income_cb_yes?).to eq true

expect(intake.cv_p2_notes_comments).to eq "Hello"
expect(intake.cv_p2_notes_comments).to eq "additional income from banana stand"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we see "Other money received during the year includes:" text anywhere in the feature specs?


textArea.addEventListener("input", function() {
if (!this.value.startsWith(prefix)) {
this.value = prefix + this.value.replace(prefix, '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this.value be nil?


textArea.form.addEventListener("submit", function() {
if (textArea.value.startsWith(prefix)) {
textArea.value = textArea.value.replace(prefix, '').trim();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can textArea.value be nil? Or is it guaranteed to be ""?

form = described_class.new(client, form_attributes)
form.save
intake.reload
end.to change(intake, :had_wages).to "yes"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how come had_wages is treated differently from all the other attributes?

Copy link
Contributor

@arinchoi03 arinchoi03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple questions, just want to understand better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants