Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Add parent company to facility claim #626

Merged
merged 1 commit into from Jun 25, 2019
Merged

Add parent company to facility claim #626

merged 1 commit into from Jun 25, 2019

Conversation

kellyi
Copy link
Contributor

@kellyi kellyi commented Jun 21, 2019 •
edited

Overview

This PR adds a parent_company field to the FacilityClaim model as a FK to Contributor, and also enables selecting, displaying, and updating that field from the list of contributorOptions.

Connects #618

Demo

Screen Shot 2019-06-22 at 10 44 55 AM
Screen Shot 2019-06-22 at 10 48 10 AM
Screen Shot 2019-06-22 at 10 47 23 AM
Screen Shot 2019-06-22 at 10 45 08 AM

Notes

Adding the new field makes the number of items displayed in the dashboard claim details page uneven, which breaks using having the two text area fields sit adjacent at the bottom of the list. We can probably wait to fix this when and if it is an actual issue.

Testing Instructions

  • get this branch, then resetdb and processfixtures and server
  • turn on claim a facility
  • visit 6543 and verify that the sidebar select dropdown filters still work as they did before
  • sign in as c2 and claim a facility. Verify that you can select a parent company from the select menu
  • sign out and sign back in as c1, then visit /dashboard/claims and click on the facility claim
  • verify that you see the parent company listed there and that it is linked to the correct profile
  • approve the claim, then sign out, and sign back in as c1
  • visit "My Facilities" and verify that you see the "Parent Company" field on the form and that you can change and save the "Parent Company" value
  • visit the facility details page for the facility you've claimed and verify that you see the parent company linked in the claim info

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 kellyi force-pushed the ki/add-parent-company-to-facility-claim branch from fb96126 to a250785 Compare June 22, 2019 14:44
@kellyi kellyi changed the title WIP Ki/add parent company to facility claim Add parent company to facility claim Jun 22, 2019
@kellyi kellyi force-pushed the ki/add-parent-company-to-facility-claim branch from a250785 to 307fa44 Compare June 22, 2019 15:03
@kellyi kellyi requested a review from jwalgran June 22, 2019 15:03
@kellyi kellyi marked this pull request as ready for review June 22, 2019 15:03
@kellyi
Copy link
Contributor Author

kellyi commented Jun 24, 2019

contributors without lists will not be selectable in the UI. If this is problematic in some way, I can create another private endpoint for the claim a facility stuff.

Turns out we do want contributors without lists to be selectable so I'll make the necessary adjustments here.

@kellyi kellyi removed the request for review from jwalgran June 24, 2019 14:59
@kellyi kellyi force-pushed the ki/add-parent-company-to-facility-claim branch 2 times, most recently from 9e33a3a to f9cca82 Compare June 24, 2019 17:58
@kellyi kellyi requested a review from jwalgran June 24, 2019 17:59
@jwalgran
Copy link
Contributor

Looking at this now.

jwalgran
jwalgran suggested changes Jun 24, 2019
View reviewed changes
Copy link
Contributor

@jwalgran jwalgran left a comment

Choose a reason for hiding this comment

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

This is a nice, straightforward implementation. I ran into a small issue with a discrepancy between user ID and contributor ID when building /profile/ links and made a suggestion regarding the field labeling on the claim form.

src/django/api/serializers.py Outdated
@@ -427,13 +427,22 @@ def get_claim_info(self, facility):
.filter(status=FacilityClaim.APPROVED) \
.get(facility=facility)

if claim.parent_company:
parent_company = {
'id': claim.parent_company.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

I ran into an issue with linking to the wrong profile page. Our /profile/ endpoint currently expects a user ID, not a contributor ID. We can fix this by adopting the strategy we use for the facility details

https://github.com/open-apparel-registry/open-apparel-registry/blob/0ac8411740f74bc8a8556dace9b0470ee556e03d/src/django/api/serializers.py#L407

In this case we would use:

'id': claim.parent_company.admin.id,

The time to fix this properly is probably when (if) we implement multi-user contributors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that -- I remember being slightly confused by this in instances when the contributor is and the contributor.admin id are the same, so it doesn't show the discrepancy. I adjusted this in efee42f.

src/app/src/util/constants.js Outdated
@@ -434,6 +434,10 @@ export const claimAFacilityFormFields = Object.freeze({
id: 'company-name',
label: 'Official name of LLC or company registered',
}),
parentCompany: Object.freeze({
id: 'parent-company',
label: 'Parent company of facility, selected from contributors list',
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider simplifying this label to "Parent company" and adding an aside. This was a bit of early feedback when reviewing the feature on a previous checkin call. Something like:

If you cannot find the parent company in this list consider inviting them to register with the Open Apparel Registry.

Screen Shot 2019-06-24 at 1 58 47 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I adjusted this on both the initial form and the update page in 364ce9d

Screen Shot 2019-06-24 at 5 39 11 PM
Screen Shot 2019-06-24 at 5 47 36 PM

@kellyi kellyi force-pushed the ki/add-parent-company-to-facility-claim branch from 364ce9d to 47fbb63 Compare June 24, 2019 22:04
@kellyi
Copy link
Contributor Author

kellyi commented Jun 24, 2019

Rebased this on develop.

@kellyi kellyi force-pushed the ki/add-parent-company-to-facility-claim branch from 47fbb63 to 22e344c Compare June 24, 2019 22:20
Add parent company to claim a facility
33cbe55 
- add parent_company field to facilityclaim model
- add & use a parent company selection on the claim a facility form
- display facility parent company on admin dashboard claim details page
- display editable facility parent company on claimed facility profile
form
- display linked facility parent company on facility details page
@kellyi kellyi force-pushed the ki/add-parent-company-to-facility-claim branch from 22e344c to 33cbe55 Compare June 24, 2019 22:55
@kellyi
Copy link
Contributor Author

kellyi commented Jun 24, 2019

Rebased on develop again & also adjusted the counter stamp on the migration.

jwalgran
jwalgran approved these changes Jun 24, 2019
View reviewed changes
Copy link
Contributor

@jwalgran jwalgran left a comment

Choose a reason for hiding this comment

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

Excellent work.

@jwalgran jwalgran assigned kellyi and unassigned jwalgran Jun 24, 2019
@kellyi
Copy link
Contributor Author

kellyi commented Jun 25, 2019

Thanks! Going to merge and then I'll run the migration on staging.

@kellyi kellyi merged commit d7740e2 into develop Jun 25, 2019
@kellyi kellyi deleted the ki/add-parent-company-to-facility-claim branch June 25, 2019 01:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jwalgran jwalgran jwalgran approved these changes

Assignees

@kellyi kellyi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kellyi @jwalgran