Update Claim a Facility claim form & profile fields #650

kellyi · 2019-07-08T17:04:33Z

Overview

add job_title and linkedin_profile fields to claim a facility claim form
enable claimants to control visibility of facility website
split facility_name field into facility_name_english and
facility_name_non_english profile fields
add facility_workers_count profile field
add facility_female_workers_percentage profile field
add facility_type field & choices for facility claim profile
add other_facility_type field for facility claim profile
adjust display order of claim info fields on facility details screen

Connects #617
Connects #634

Demo

Notes

There are a bunch of migrations here but I'll squash them when the other PRs are merged in -- so I didn't bother renaming anything.

I made discrete followup issues for handling

product type Add Product Type field to claim a facility profile #651
production type Add Production Type field to claim a facility profile #652
certifications Add Certifications to claimed facility profile #653
membership bodies Add Affiliations field to claim a facility profile #654

Testing Instructions

get this branch, then resetdb & processfixtures
sign in and turn on claim a facility
try to claim a facility and verify that you see the new claim form fields, with validation
sign in as an admin and view the claim, verifying that you see the new claim form fields displayed in the dashboard
approve the claim, then sign back in as the user who has claimed the facility and visit the claim edit profile page
verify that you can update and save data in the new inputs
visit the facility details page and verify that the new fields are visible (if they aren't null) and that the order matches the order requested in Claim a facility feedback #617

Checklist

fixup! commits have been squashed
CI passes after rebase
CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

kellyi · 2019-07-09T19:54:43Z

Note from #647 (comment)

As part of this pull request add a commit to:

Make the website textbox be red when the URL is incorrect in the Claim application form
Make the website clickable when reviewing a claim as an admin

jwalgran · 2019-07-09T20:50:04Z

I will hold off on reviewing this until the migration and code conflicts are resolved.

kellyi · 2019-07-09T21:59:06Z

Rebased this on develop and also made some adjustments

we now use save rather than update for the claim update PUT endpoint so that things work with django-simple-history
I made the website link clickable on the dashboard and also added the validation styling on the claim form website input

@jwalgran I believe this is ready for review!

jwalgran · 2019-07-10T18:16:32Z

Looking at this now.

kellyi · 2019-07-10T19:27:13Z

I pushed d67149d which is a new commit to adjust the facility type field options to match the new request on #634

kellyi · 2019-07-10T20:45:41Z

Rebased on develop

jwalgran

Thanks for breaking up this big change into more easily reviewable chunks. There are some problems to resolve and I made some suggestions.

jwalgran · 2019-07-10T18:39:03Z

src/app/src/components/DashboardClaimsDetails.jsx

                <InfoSection
                    label="Facility Parent Company"
                    value={
                        (() => {
                            const parentCompanyName = get(data, 'facility_parent_company.name', null);

                            if (!parentCompanyName) {
-                                return null;
+                                return '';


Curious about this change from null to ''. Is it to prevent rendering the string "null?"

I believe this was to remedy a propTypes warning but is otherwise a noop.

jwalgran · 2019-07-10T18:46:52Z

src/django/api/models.py

-        help_text='The editable facility name for this claim.')
+        help_text='The editable official English facility name for the claim.',
+        verbose_name='facility name in English')
+    facility_name_non_english = models.CharField(


#634 calls for this field to be "Official facility name (native language)." Unless this label change was specifically requested I do think that "native language" is a better descriptor than "non-English"

Sounds good. I'll update the field names throughout.

Done in 65b7fa4

jwalgran · 2019-07-10T18:48:35Z

src/app/src/components/ClaimedFacilitiesDetails.jsx

@@ -27,7 +27,10 @@ import COLOURS from '../util/COLOURS';
 import {
    fetchClaimedFacilityDetails,
    clearClaimedFacilityDetails,
-    updateClaimedFacilityName,
+    updateClaimedFacilityNameEnglish,
+    updateClaimedFacilityNameNonEnglish,


Consider "non-english" → "native language"

Done in 65b7fa4

jwalgran · 2019-07-10T18:48:43Z

src/app/src/components/ClaimedFacilitiesDetails.jsx

+                    disabled={updating}
+                />
+                <InputSection
+                    label="Facility name (non-English language)"


Consider "non-english" → "native language"

Done in 65b7fa4

jwalgran · 2019-07-10T18:48:50Z

src/app/src/components/ClaimedFacilitiesDetails.jsx

@@ -229,14 +232,17 @@ function ClaimedFacilitiesDetails({
    data,
    getDetails,
    clearDetails,
-    updateFacilityName,
+    updateFacilityNameEnglish,
+    updateFacilityNameNonEnglish,


Consider "non-english" → "native language"

Done in 65b7fa4

jwalgran · 2019-07-10T18:50:34Z

src/django/api/views.py

-                facility_name=request.data.get('facility_name'),
+                facility_name_english=request.data
+                .get('facility_name_english'),
+                facility_name_non_english=request.data


Consider "non-english" → "native language"

Done in 65b7fa4

jwalgran · 2019-07-10T19:11:56Z

src/app/src/components/FacilityDetailSidebarClaimedInfo.jsx

                label="Minimum Order"
            />
            <ClaimInfoSection
-                value={facility.facility_average_lead_time}
+                value={facility.average_lead}


Was this field incorrectly named and we are now fixing it in passing along with reordering the fields?

Yeah, that's right. It's called average_lead in the geojson properties but it was just silently absent here.

jwalgran · 2019-07-10T19:14:02Z

src/django/api/views.py

-                office_country_code=request.data.get('office_country_code'),
-                office_phone_number=request.data.get('office_phone_number'),
-                office_info_publicly_visible=request.data
+            claim_to_update = FacilityClaim \


#649 includes a data-driven version of this change in db35741. Consider incorporating that here to prevent a difficult merge.

Good suggestion. I adjusted this in

eaf286f

and

6e649a0

The latter commit adjusts the order of how the non-dictionary settings get saved in order to ensure we save those fields too.

jwalgran · 2019-07-10T19:33:29Z

src/app/src/components/ClaimFacilityContactInfoStep.jsx

+                    </Typography>
+                </InputLabel>
+                <TextField
+                    error={isEmpty(jobTitle)}


The next button is still enabled when this field is empty

I fixed this in 4c3b188

jwalgran · 2019-07-10T20:48:33Z

src/django/api/views.py

+                .data \
+                .get('facility_average_lead_time')
+
+            claim_to_update.facility_workers_count = request \


If I leave either this field of the female worker percentage field blank saving crashes with

ValueError: invalid literal for int() with base 10: ''

Fixed this in 6449b9f in two parts:

prevent the client from sending non-int values for these fields

if the api encounters non-int values here, they are ignored and the fields are set to None

kellyi · 2019-07-11T16:07:53Z

@jwalgran this is ready for another review!

jwalgran · 2019-07-11T16:10:53Z

Looking at this now.

kellyi · 2019-07-11T16:16:44Z

Commit message for cf92229
is incorrect. the isInteger || isInt thing is to check whether the value is an int or can be cast to an int.

jwalgran

Fixes look good and work well. Thanks. Mind the squash.

kellyi · 2019-07-11T16:33:49Z

Fixes look good and work well. Thanks. Mind the squash.

Awesome, thanks!

- add fields for facility claim form and claimed facility profile - adjust display order for claimed facility details page - replace .update with .save in facility claim details endpoint

kellyi force-pushed the ki/add-new-claim-facility-fields branch 3 times, most recently from 89f9537 to b25e320 Compare July 8, 2019 20:27

kellyi mentioned this pull request Jul 8, 2019

Fields for facility profile #634

Closed

kellyi force-pushed the ki/add-new-claim-facility-fields branch from fe38fcb to 8d93c74 Compare July 8, 2019 21:26

kellyi changed the title ~~WIP Add job title and linkedin profile to claim form~~ Update Claim a Facility claim form & profile fields Jul 9, 2019

kellyi requested a review from jwalgran July 9, 2019 02:34

kellyi assigned jwalgran Jul 9, 2019

kellyi marked this pull request as ready for review July 9, 2019 02:34

kellyi force-pushed the ki/add-new-claim-facility-fields branch 3 times, most recently from 6b41b54 to df94afc Compare July 9, 2019 21:57

kellyi mentioned this pull request Jul 10, 2019

Add facility field changes to contributor notification emails #649

Merged

3 tasks

kellyi force-pushed the ki/add-new-claim-facility-fields branch from d67149d to e9495f9 Compare July 10, 2019 20:42

jwalgran reviewed Jul 10, 2019

View reviewed changes

jwalgran assigned kellyi Jul 10, 2019

kellyi force-pushed the ki/add-new-claim-facility-fields branch from e9495f9 to b56e684 Compare July 11, 2019 14:01

kellyi force-pushed the ki/add-new-claim-facility-fields branch from 6e649a0 to cf92229 Compare July 11, 2019 16:12

jwalgran approved these changes Jul 11, 2019

View reviewed changes

Add fields for claim form and profile

2dcd09b

- add fields for facility claim form and claimed facility profile - adjust display order for claimed facility details page - replace .update with .save in facility claim details endpoint

kellyi force-pushed the ki/add-new-claim-facility-fields branch from cf92229 to 2dcd09b Compare July 11, 2019 16:40

kellyi merged commit a1c506f into develop Jul 11, 2019

kellyi deleted the ki/add-new-claim-facility-fields branch July 11, 2019 16:48

jwalgran mentioned this pull request Oct 28, 2019

Data fields for claiming a facility #533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Claim a Facility claim form & profile fields #650

Update Claim a Facility claim form & profile fields #650

kellyi commented Jul 8, 2019 •

edited

kellyi commented Jul 9, 2019 •

edited

jwalgran commented Jul 9, 2019

kellyi commented Jul 9, 2019

jwalgran commented Jul 10, 2019

kellyi commented Jul 10, 2019

kellyi commented Jul 10, 2019

jwalgran left a comment

jwalgran Jul 10, 2019

kellyi Jul 11, 2019

jwalgran Jul 10, 2019

kellyi Jul 11, 2019

kellyi Jul 11, 2019

jwalgran Jul 10, 2019

kellyi Jul 11, 2019

jwalgran Jul 10, 2019

kellyi Jul 11, 2019

jwalgran Jul 10, 2019

kellyi Jul 11, 2019

jwalgran Jul 10, 2019

kellyi Jul 11, 2019

jwalgran Jul 10, 2019

kellyi Jul 11, 2019

jwalgran Jul 10, 2019

kellyi Jul 11, 2019

jwalgran Jul 10, 2019

kellyi Jul 11, 2019

jwalgran Jul 10, 2019

kellyi Jul 11, 2019

kellyi commented Jul 11, 2019

jwalgran commented Jul 11, 2019

kellyi commented Jul 11, 2019

jwalgran left a comment

kellyi commented Jul 11, 2019

Update Claim a Facility claim form & profile fields #650

Update Claim a Facility claim form & profile fields #650

Conversation

kellyi commented Jul 8, 2019 • edited

Overview

Demo

Notes

Testing Instructions

Checklist

kellyi commented Jul 9, 2019 • edited

jwalgran commented Jul 9, 2019

kellyi commented Jul 9, 2019

jwalgran commented Jul 10, 2019

kellyi commented Jul 10, 2019

kellyi commented Jul 10, 2019

jwalgran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellyi commented Jul 11, 2019

jwalgran commented Jul 11, 2019

kellyi commented Jul 11, 2019

jwalgran left a comment

Choose a reason for hiding this comment

kellyi commented Jul 11, 2019

kellyi commented Jul 8, 2019 •

edited

kellyi commented Jul 9, 2019 •

edited