-
Notifications
You must be signed in to change notification settings - Fork 7
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
Reapply site address question changes with some modifications #7511
Reapply site address question changes with some modifications #7511
Conversation
data-hub-frontend
|
Project |
data-hub-frontend
|
Branch Review |
feature/CLS2-1175-site-address-field-changes
|
Run status |
|
Run duration | 08m 05s |
Commit |
|
Committer | Oliver Roberts |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
0
|
|
4
|
|
0
|
|
74
|
View all changes introduced in this branch ↗︎ |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7511 +/- ##
=======================================
Coverage 88.57% 88.57%
=======================================
Files 1144 1144
Lines 17789 17798 +9
Branches 5097 5105 +8
=======================================
+ Hits 15756 15765 +9
Misses 2033 2033 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to disregard the comment if you know it won't work like that
src/client/modules/Investments/Projects/Details/EditProjectRequirements.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, for the parts in the transformers that coverage highlighted, are they missing tests? Could a unit test be added for them?
Good spot. I would argue if (siteAddressIsCompanyAddressBool === true && ukCompany) {
return { is covered by it('should submit site address is company address is true and populate address fields when user selects yes and project has uk company', () => { In regards to the other missed line, I've added a test for the |
Description of change
Reapplies PR #7486 (commit 382debc) with some modifications. These include:
actual_uk_regions
question and unnesting it(site_address_is_company_address == True && uk_company is not None)
is FalseUpdated API PR can be found here.
Test instructions
For projects that have:
site_address_is_company_address === null
: the radio buttons should be unselected. If no modifications are made, the frontend will set the address fields to existing values upon submission.site_address_is_company_address === true && !uk_company)
: the yes option will be selected and show some help text prompting users to select a UK recipient company. Upon submission, the frontend will setsite_address_is_company_address === true
the address fields to existing values.site_address_is_company_address === true && uk_company)
: the yes option will be selected and show the UK recipient company address fields. Upon submission, the frontend will setsite_address_is_company_address === true
the address fields to the UK company address.site_address_is_company_address === false
: the no option will be selected and address fields will appear for manual entry. Upon submission, the frontend will setsite_address_is_company_address === false
and the address fields to the values entered.The
actual_uk_regions
field is now separate/unnested and should behave as usual.Screenshots
Checklist