Enable users to update approved claimed facility profiles & display claim info in the details sidebar #575

kellyi · 2019-06-11T20:39:31Z

Overview

This PR enables users with approved facility claims to edit these claims using a form on the /claimed/claimID route and also updates the facility details sidebar to display that info beneath the existing facility data.

I spun off the work of sending emails to a separate card.

I also spun off adding some multiselect tag fields to a separate card, since we don't really yet know the set of tags for these fields. It occurred to me that we can also remove & defer adding the minimum_order_quantity and average_lead_time fields, too, for now since users can currently just include this info in a free form way in the facility_description field.

Connects #518

Demo

Testing Instructions

get this branch, then resetdb, processfixtures, switch on the claim a facility and create a superuser
sign up for an account through the UI, then fill out a claim a facility form
sign in as the superuser and approve the claim
sign back in as the other user and visit the OAR ID for the facility you have claimed. Clicking on the claim badge will now take you to your approved claim (if signed out, clicking on the badge will go to a not-yet-existing /about/claimedfacilities route
verify that the claimed facility profile form loads and that you can edit and save the fields and that those changes get reflected accurately in the facility details UI

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-06-12T12:27:00Z

I am spinning off the multiselect fields for "certifications", "product types", and "production types" to #576 since we may need to spend some time cataloging the set of options before adding those.

kellyi · 2019-06-12T12:36:37Z

Remaining work here:

link details verified badge to /about/claimedfacilities for users who have not claimed the facility and to /claimed/ID for users who have claimed the facility
update the user serializer to send back a list of the OAR IDs for a user's pending & approved facility claims
change country inputs to react select boxes

kellyi · 2019-06-12T13:17:28Z

I spun off emailing contributors on profile changes to #578

jwalgran

The form looks sharp and the core flow is working well. I found a few items that need to be addressed and suggested some follow up issues.

jwalgran · 2019-06-13T00:07:02Z

src/app/src/components/FacilityDetailSidebarClaimedInfo.jsx

+            />
+
+            <ClaimInfoSection
+                label="Claimed Facility Description"


The wireframes do not have a "Claimed Facility ..." prefix on these fields. Consider dropping the prefix to reduce some visual noise.

Done in 3c0b219

I also added a header to indicate what the info represents. I think we will also need to add some kind of disclaimed or link to the about page here.

Disclaimer message issue is #588

Consider dropping the "Facility" prefix too. All the information in the sidebar is about the facility.

Dropped the "Facility" prefix in e374b2d

jwalgran · 2019-06-13T00:08:45Z

src/app/src/components/FacilityDetailSidebar.jsx

@@ -146,20 +139,38 @@ class FacilityDetailSidebar extends Component {
                            {data.properties.name}
                        </h3>
                        <p className="panel-header__subheading notranslate">
-                            {data.properties.address} - {data.properties.country_name}
+                            {data.properties.address} -{' '}


Why did we need to change this line? {' '} is not something we see regularly.

I think this got added by running Prettier on this file in my editor; it's how Prettier preserves displayed space chars when breaking text across lines. I can put this all on a single line, though, and remove the {' '}.

Done in 1bf02ca

jwalgran · 2019-06-13T00:14:08Z

src/app/src/components/FacilityDetailSidebarClaimedInfo.jsx

+
+            <ClaimInfoSection
+                label="Claimed Facility Address"
+                value={`${facility.address || ' '} ${facility.country || ' '}`}


facility.country is the country code. We probably want the country name in this situation.

But...

I want to double check that we want to have a country field on the form at all. If the country on a row is wrong, it should be corrected at higher level (the facility, or even the facility list item)

Good point. For now I'll remove the facility country selection and field altogether from the form & model (but leave the one for office country). If we decide we do want to allow changing a facility claim country, then we can add it back later.

Removed in 9c3296b

jwalgran · 2019-06-13T00:22:27Z

src/app/src/components/FacilityDetailSidebarClaimedInfo.jsx

+                label="Claimed Facility Phone Number"
+            />
+            <ClaimInfoSection
+                value={facility.minimum_order}


The model field is minimum_order_quantity but in some places we are using minimum_order. I can't save a value in the the minimum order field and I think the naming inconsistency is the reason.

Fixed in 9c3296b

jwalgran · 2019-06-13T00:45:26Z

src/django/api/serializers.py

+            return []
+
+        try:
+            contributor = Contributor.objects.get(admin=user)


Consider using some features of the Django ORM to simplify this a bit

return (FacilityClaim.objects .filter(status=FacilityClaim.APPROVED) .filter(contributor=user.contributor) .values_list('facility__id', flat=True))

Thanks for the suggestion! This is in 5d2e4de

jwalgran · 2019-06-13T03:00:42Z

src/app/src/components/FacilityDetailSidebar.jsx

+                                    data={data.properties.claim_info}
+                                />
+                            </ShowOnly>
+                        </FeatureFlag>
                        <div className="control-panel__group">
                            <div style={detailsSidebarStyles.linkSectionStyle}>
                                <a


The wireframes show conditionally hiding these links if the logged in contributor has claimed the facility, since the do not apply to the claimant. Instead, the wireframes have a link to the profile editing page above the satellite image. Since we already have access to facilityIsClaimedByCurrentUser we should consider implementing this link swapping.

Handled in be9749d

Not my claimed facility:

My claimed facility:

Note that the verified badge will already also conditionally link to the facility claim form if it is my claimed facility.

jwalgran · 2019-06-13T03:12:02Z

src/app/src/components/ClaimedFacilitiesDetails.jsx

+                    disabled={updating}
+                />
+                <InputSection
+                    label="Email"


Consider creating a follow up issue to add email address validation to this field.

This should be trivial to incorporate here, so I can just add it in this PR. I think we want to validate isEmpty(email) || isEmail(email) for this field, and probably disable the save button concurrently.

Added in 9c3296b

jwalgran · 2019-06-13T03:15:06Z

src/app/src/components/ClaimedFacilitiesDetails.jsx

+                    onSwitchChange={updateFacilityPhoneVisibility}
+                />
+                <InputSection
+                    label="Website"


Consider creating a follow up issue to add url validation to this field and to render it as an active link in the sidebar (using the same conditional "http://" appending that we use on the contributor profile page.

This is/will be #587

jwalgran · 2019-06-13T03:24:02Z

src/app/src/components/ClaimedFacilitiesDetails.jsx

+                    onChange={updateOfficePhone}
+                    disabled={updating}
+                />
+                {errorUpdating && (


Consider opening an issue to use full_clean on the model to trigger Django's model validation and generate helpful error messages that we can show to the claimant. We do this when parsing uploaded files https://github.com/open-apparel-registry/open-apparel-registry/blob/8fdc09c4d6cac5f4aefc3a313e9c04d84e5ab49f/src/django/api/processing.py#L127

We are currently returning 500 when the claimant enters too many characters into a field.

We could also make use of maxlength to prevent these types of errors.

I made #589 for handling it. I think we could also use the existing serializer to check somehow.

jwalgran · 2019-06-13T03:27:28Z

src/app/src/components/ClaimedFacilitiesDetails.jsx

+                        color="primary"
+                        disabled={updating}
+                    >
+                        Submit


Consider "Save" or "Save Changes" instead of "Submit"

Also consider showing some "toast" or similar if there is a 200 response. Right now there is no indication that the save succeeded.

"Save" is in 86904d2

toast is in cd8cc08

kellyi · 2019-06-13T15:23:22Z

@jwalgran I believe I've addressed all the feedback above, so this is ready for another look!

I updated the new migration, so testing will need to start from resetdb again.

kellyi · 2019-06-13T15:25:18Z

I'm going to squash this down again.

jwalgran · 2019-06-13T15:54:51Z

Thanks for all the revisions. Taking another look.

jwalgran · 2019-06-13T16:06:18Z

Running into a prop type error when selecting a facility on the map

Warning: Failed prop type: The prop `data` is marked as required in `FacilityDetailsSidebarClaimedInfo`, but its value is `null`.
    in FacilityDetailsSidebarClaimedInfo (at FacilityDetailSidebar.jsx:221)
    in FacilityDetailSidebar (created by Connect(FacilityDetailSidebar))
    ...

kellyi · 2019-06-13T16:09:16Z

Fixed the propType error in 381817e

jwalgran

This is working perfectly. Thanks for all the time put into this implementation.

kellyi · 2019-06-13T16:16:47Z

Awesome, thanks for all your help with this!

- update model and add migration for editable facility claim profile fields - add Redux actions & React components for editable claimed facility profile form - adjust FacilityDetailsSerializer to return facility claim info conditionally - add FacilityDetailSidebarClaimedInfo component to display claim info

Ensure dashboard claims are always displayed in reverse order by adding -id sort.

kellyi force-pushed the ki/claim-a-facility-profile branch 3 times, most recently from 26bbe33 to 571b60b Compare June 12, 2019 01:12

kellyi mentioned this pull request Jun 12, 2019

Add certification, product types, and production types fields to facility claim form & details display #576

Closed

kellyi mentioned this pull request Jun 12, 2019

Email contributors when a facility claim has been approved #577

Closed

kellyi mentioned this pull request Jun 12, 2019

Email contributors when a facility claim profile has been updated #578

Closed

kellyi mentioned this pull request Jun 12, 2019

DRAFT claimed badge is a button but it has no action #579

Closed

kellyi force-pushed the ki/claim-a-facility-profile branch from fc67a70 to 1e6b6b7 Compare June 12, 2019 17:58

kellyi changed the title ~~WIP claimed facility profile~~ Enable users to update approved claimed facility profiles & display claim info in the details sidebar Jun 12, 2019

kellyi force-pushed the ki/claim-a-facility-profile branch from 1e6b6b7 to 91e8732 Compare June 12, 2019 18:10

kellyi requested a review from jwalgran June 12, 2019 18:10

kellyi assigned jwalgran Jun 12, 2019

kellyi marked this pull request as ready for review June 12, 2019 18:10

kellyi force-pushed the ki/claim-a-facility-profile branch 2 times, most recently from 9092aa2 to 641f248 Compare June 13, 2019 00:36

jwalgran reviewed Jun 13, 2019

View reviewed changes

jwalgran assigned kellyi Jun 13, 2019

kellyi force-pushed the ki/claim-a-facility-profile branch from d53d5c4 to 381817e Compare June 13, 2019 16:11

jwalgran approved these changes Jun 13, 2019

View reviewed changes

kellyi force-pushed the ki/claim-a-facility-profile branch from 381817e to 6b5365c Compare June 13, 2019 16:18

Set -id order for dashboard claims page

b6027e1

Ensure dashboard claims are always displayed in reverse order by adding -id sort.

kellyi force-pushed the ki/claim-a-facility-profile branch from 6b5365c to b6027e1 Compare June 13, 2019 16:21

kellyi merged commit de69c3b into develop Jun 13, 2019

kellyi deleted the ki/claim-a-facility-profile branch June 13, 2019 16:27

kellyi mentioned this pull request Jun 13, 2019

Show a list of facilities successfully claimed by the current contributor #573

Merged

3 tasks

jwalgran mentioned this pull request Oct 28, 2019

Data fields for claiming a facility #533

Closed

Enable users to update approved claimed facility profiles & display claim info in the details sidebar #575

Enable users to update approved claimed facility profiles & display claim info in the details sidebar #575

Conversation

kellyi commented Jun 11, 2019 • edited Loading

Overview

Demo

Testing Instructions

Checklist

kellyi commented Jun 12, 2019 • edited Loading

kellyi commented Jun 12, 2019 • edited Loading

kellyi commented Jun 12, 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

kellyi Jun 13, 2019 • edited Loading

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 Jun 13, 2019

kellyi commented Jun 13, 2019

jwalgran commented Jun 13, 2019

jwalgran commented Jun 13, 2019

kellyi commented Jun 13, 2019 • edited Loading

jwalgran left a comment

Choose a reason for hiding this comment

kellyi commented Jun 13, 2019

kellyi commented Jun 11, 2019 •

edited

Loading

kellyi commented Jun 12, 2019 •

edited

Loading

kellyi commented Jun 12, 2019 •

edited

Loading

kellyi Jun 13, 2019 •

edited

Loading

kellyi commented Jun 13, 2019 •

edited

Loading