Use admin IDs to resolve entities during import, add validation for conflicts in entity maps

[hancush]

Overview

This PR:

• Adds a new EntityMap class to keep track of units and people present in the import
• Logs when UUIDs have more than one associated name and when names have more than one associated UUID (although the latter is not always an error, the SFM team would appreciate the warnings, because sometimes more than one UUID is minted for the same entity by mistake)
• Updates the import throughout to resolve entities based on their UUID, rather than their name. This is necessary because the current approach of using a name will collapse distinct entities sharing the same name under one UUID, which has serious implications for data integrity.
• Includes test data updates c/o @tlongers in Update test fixtures to handle differences between UUIDs and entity name values in person and units #778
• Documents and clarifies the existing sourcing test and adds a new sourcing test validating that all data points have evidence
• Adds test for the new resolution and logging behavior

Connects external issue.

Description of logging behavior

Organizations are referenced by name and UUID in the units sheet, as well as the persons and events sheets. Similarly, people are referenced by name and UUID in the persons and events sheets.

The way I've implemented logging will gather references to a given org or person across all sheets. If there is more than one distinct name value, it will log an error for all rows referencing that UUID, so [SFM has] an audit trail of how the entity is represented across the import. Hopefully, this makes it easier to spot which record is not like the others and should be fixed.

So, say the units sheet has an organization with the UUID 1 named Organization A, and the persons sheet says someone is posted to an organization with the UUID 1 named Organisation A. You'd get a record in the organization errors that looks like:

 ${LINE_NUMBER},"Got multiple name values for organization UUID ""1"". Current row contains value ""Organization A"" in column ""unit:name"""

And one in the person errors that looks like:

${LINE_NUMBER},"Got multiple name values for organization UUID ""1"". Current row contains value ""Organisation A"" in column ""person:posting_unit_name"""

Ditto for more than one distinct UUID for a given name value.

Testing Instructions

• Confirm CI passes

[hancush]

These attributes are, in fact, all sourced. (Violation records come with a single source that the importer associates with each data point.) The complex field module expects sourced data points to be declared sourced and will not return any sources – even if they exist – if data points are not marked as such.

This change makes sourcing more explicit and allows me to use get_sources in the new test confirming all data points have evidence. Unfortunately, the source creation form behaves differently in that it only expects and creates a source for description, so this change breaks those tests (and the UI).

Per @tlongers, this UI is no longer in use. I have marked the failing tests for skipping, and will open a separate issue to clean up the unused code.

[hancush]

None of these sets changed – I just put each item on its own line and sorted them alphabetically, for easier reading.

[hancush]

The new test data does not conform with the pattern of source creation we used when we wrote the test. Explicitly query for the sources created for the purpose of this test, so new test data may be added without breaking this test.

[hancush]

We have a lot of ways of reporting the same error. These changes add additional log messages to check for. I'll open a separate issue to unify our logging language.

[hancush]

New test.

[hancush]

Existing test, renamed, documented, and reformatted.

[fgregg]

Very nice cleanup

[fgregg]

Seems like this might be a bit easier to reason about if you implemented this as a nested dict

__init__(self):
    _d = {}
...
if key not in self:
    self._d[key] = {}

location = (column, row, sheet)
if value not in self[key]:
    self_d[key][value] = [location]
else:
    self._d[key][value].append(location)

return self

if you need the data back in a particular way you are getting it now you could
implement this dunder method

def __getitem__(self, key):
    return [(k,) + values for k, values in self._d[key].items()]

[hancush]

Hmm, I'm finding this suggestion much harder to parse. I originally implemented the map as an object with a dictionary attribute here, e182853, but I found that most times I accessed self, I was accessing self.map (the dictionary), so it felt more direct for EntityMap to be a dictionary itself. Is this a change you require?

[fgregg]

i think you effectively do have a nested structure since the logic, as, I understand it is to have

key
|_ value
    |_ location

which you sometimes want to transpose to another nested structure:

value
|_ key
    |_ location

i think it's going to be easier to work with this in the future if your data structure reflects that nesting.

so, i do want to see an implementation that has that nesting.

I do not require that you implement this with a dictionary attribute. that's only necessary if you need to flattened results back as in the example __getitem__ if you don't need that flattening, you could do.

if key not in self:
    self[key] = {}

location = (column, row, sheet)
if value not in self[key]:
    self[key][value] = [location]
else:
    self[key][value].append(location)

return self

[hancush]

Thanks for clarifying! I missed the substance of your first comment. 😅 Refactored in ad9d10b.

[fgregg]

looking good. one slight improvement possible.

[fgregg]

this can be if len(values) > 1 since values (as keys to a dictionary) are guaranteed to be unique

[hancush]

Nice catch, thank you and donezo.