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

Add product and production type to claimed facility profile #680

Merged
merged 4 commits into from Jul 16, 2019

Conversation

jwalgran
Copy link
Contributor

@jwalgran jwalgran commented Jul 16, 2019

Overview

Brief description of what this PR does, and why it is needed.

Connects #652

Demo

Optional. Screenshots, curl examples, etc.

Notes

Optional. Ancillary topics, caveats, alternative strategies that didn't work out, anything else.

Testing Instructions

  • How to test this PR
  • Prefer bulleted description
  • Start after checking out this branch
  • Include any setup required, such as bundling scripts, restarting services, etc.
  • Include test case, and expected output

Checklist

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

Facility claimants will be able to add any number of arbitrary product types and
production types to their facility profile. These new tables act as "seed lists"
for multi-select lists.
These array fields will hold the arbitrary collection of values entered by the
claimant.
@kellyi kellyi force-pushed the jcw/add-product-and-production-type branch from 85a92b7 to 0b6bb8f Compare July 16, 2019 19:40
@kellyi kellyi self-requested a review July 16, 2019 19:41
);
}

window.console.warn('isCreatable && !isMultiSelect case is not yet implemented');
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not currently have a select box with these settings here but I left open the possibility that we could.

(choice, choice)
for choice
in union_of_seeds_and_values
]
Copy link
Contributor

Choose a reason for hiding this comment

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

I revised how these serializers were working to make them return a list of tuples like [("Accessories", "Accessories")] etc. This matches how the other choice serializers work here which makes it easier to reuse the existing mapDjangoChoiceTuples stuff in the client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@kellyi kellyi force-pushed the jcw/add-product-and-production-type branch 2 times, most recently from e2d5094 to ff60fb4 Compare July 16, 2019 20:13
@kellyi kellyi marked this pull request as ready for review July 16, 2019 20:14
Copy link
Contributor

@kellyi kellyi left a comment

Choose a reason for hiding this comment

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

I tested this out including my fixup and it is working well! I'm going to make a note of a place where I'd like a check on my reasoning in the serializer.

new_value
for new_value
in new_values or []
]
Copy link
Contributor

@kellyi kellyi Jul 16, 2019

Choose a reason for hiding this comment

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

I'd like to check my reasoning here a little bit:

  • the values_list call for seeds will always return a list because of our data migration
  • the values_list call for facility_product_types will return None if no custom types have been entered
  • the values_list call for facility_product_types returns a list of lists when it does return values

So we check whether new_values.exists() here to see whether it is not None and if it is not, we remove one level of nesting by getting the 0th index. If it is None, that's okay because the or [] thing in the list comprehension above will just make values into an empty list.

Subsequently we union the seeds and custom values together, then sort them alphabetically before making the tuples to send back as choices.

(Same logic for production types, btw). Does this seem reasonable? I tested it out and it worked both when there were no existing facility_product_type values and when there were some existing values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this reasoning by adding products to 2 different profiles. It does not look like we want new_values[0]

>>> new_values = FacilityClaim.objects.all().values_list('facility_product_types', flat=True)
>>> new_values
<QuerySet [['Bedskirts', 'Foo'], ['Chaps', 'Foo', 'Spats']]>
>>> new_values[0]
['Bedskirts', 'Foo']

We can combine set and itertools.chain to get a set that we can union with our seeds.

>>> from itertools import chain
>>> set(chain.from_iterable(new_values))
{'Bedskirts', 'Foo', 'Spats', 'Chaps'}

Going to rework this if you agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds fine to me -- the main thing I was interested in doing is to make sure we return the list of choices formatted like [('A', 'A')] from the API since that's generally what the client expects for these. Feel free to adjust how we get there, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

if (isCreatable && isMultiSelect) {
const [
optionsInExistingSet,
newlyCreatedOptionsPrime,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed that I made a naming mistake here by leaving Prime on the end of this, from when I was trying out a couple ways to generate these partitions. I guess it doesn't hurt anything to leave it but if we'd like we can adjust the name to be just newlyCreatedOptions.

@@ -283,4 +283,6 @@ export const approvedFacilityClaimPropType = shape({
facility_types: arrayOf(arrayOf(string)).isRequired,
affiliation_choices: arrayOf(arrayOf(string)).isRequired,
certification_choices: arrayOf(arrayOf(string)).isRequired,
product_type_choices: arrayOf(string).isRequired,
production_type_choices: arrayOf(string).isRequired,
Copy link
Contributor

@kellyi kellyi Jul 16, 2019

Choose a reason for hiding this comment

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

Just noticed that these need to be arrayOf(arrayOf(string)) now.

A claimant can enter arbitrary product or production types, so we use the
`Creatable` version of `react-select` to collect them on the profile form. To
encourage reuse we populate the select boxes with a combination of a "seed" list
and all the arbitrary values that have been added across all claims.
@jwalgran jwalgran force-pushed the jcw/add-product-and-production-type branch from ff60fb4 to f6581b1 Compare July 16, 2019 21:47
@jwalgran jwalgran changed the title Add product and production type Add product and production type to claimed facility profile Jul 16, 2019
@jwalgran
Copy link
Contributor Author

Thanks for helping me finish this up. Sorry your contribution was lost to rebase.

@jwalgran jwalgran merged commit 605fb31 into develop Jul 16, 2019
@jwalgran jwalgran deleted the jcw/add-product-and-production-type branch July 16, 2019 21:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants