Conversation
The ExtendedFields from FacilityListItems will be created in two steps: firstly, they will be initialized with a null facility field when parsing the list item, at which time the list item doesn't have an assigned facility; secondly, they will be assigned to a facility in the matching phase, when the list item is linked to a facility. To allow the first step, the facility field is now nullable.
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.
Everything is working as described. I like the design of butting a set of helper function in an extended_fields
module. I do want to discuss the decision to make ExtendedField for list item core fields.
From the issue:
When parsing a FacilityListItem, look for well known extended field keys in the submitted list item and create ExtendedField rows.
The design in my head when I wrote this was that we would only create number_of_workers and native_language_name extended fields for submitted list items, so I was suprised to see country, name, and address. Name and address do make sense for claims. Did you opt to also create them for list items for consistency?
If we do go ahead with creating extended field rows for all core fields, we will need to add a data migration to create the extended field records for every list item created before this feature. That could be created in a follow up issue/PR if appropriate.
src/django/api/extended_fields.py
Outdated
def create_extendedfield(field, field_value, item, contributor): | ||
if field_value is not None and field_value != "": | ||
if field == ExtendedField.NUMBER_OF_WORKERS: | ||
field_value = {"min": field_value, "max": field_value} |
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.
We want to allow a contributor to specify a range. Consider adding some regex processing to allow flexibly extracting the min and max
>>> value = '123'
>>> [int(x) for x in re.findall(r'([0-9]+)', value.replace(',', ''))]
[123]
>>> value = '1,00 to 1,500'
>>> [int(x) for x in re.findall(r'([0-9]+)', value.replace(',', ''))]
[100, 1500]
src/django/api/extended_fields.py
Outdated
field_value = getattr(claim, claim_field) | ||
if field_value is not None and field_value != "": | ||
if extended_field == ExtendedField.NUMBER_OF_WORKERS: | ||
field_value = {"min": field_value, "max": field_value} |
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.
See the earlier suggestion about using a regex to extract min and max from the value
I've updated the number_of_workers to use the suggested regex processing and format the min and max found values. |
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.
Changes work well. 👍
Users can submit facility list items individually through the API or via a list. During parsing, we search for known extended fields and create ExtendedFields as applicable. Later, we assign a facility id to the ExtendedFields if one has been assigned to the list item (for MATCHED and NEW_FACILITY statuses). For POTENTIAL_MATCH statuses, the item has not yet been assigned to a facility. When the user confirms or rejects the match, the list item and ExtendedFields are both linked to a facility. Extended facility details can also be submitted via facility claim. In this case, the facility has already been created, so the ExtendedFields are linked to a facility immediately. A user can later update the details supplied with their claim; in this case, changed fields are updated, and removed fields are deleted.
bf20a4e
to
1130375
Compare
Thank you for reviewing! |
Overview
Users can submit facility list items individually through the API or
via a list. During parsing, we search for known extended fields and create
ExtendedFields as applicable. Later, we assign a facility id to the
ExtendedFields if one has been assigned to the list item (for MATCHED
and NEW_FACILITY statuses).
For POTENTIAL_MATCH statuses, the item has not yet been assigned to a
facility. When the user confirms or rejects the match, the list item and
ExtendedFields are both linked to a facility.
Extended facility details can also be submitted via facility claim. In
this case, the facility has already been created, so the ExtendedFields
are linked to a facility immediately. A user can later update the
details supplied with their claim; in this case, changed fields are
updated, and removed fields are deleted.
Connects #1493
Notes
In the issue, we list
promote
as a location where a ExtendedField might need to be updated. However, while the facility is updated to point to a different FacilityListItem, the FacilityListItem (which the ExtendedField is linked to) still points to the same facility, so it doesn't seem that any changes are needed to the ExtendedField in response.Testing Instructions
./scripts/manage migrate
./scripts/manage batch_process --list-id 16 --action parse
Checklist
fixup!
commits have been squashed