Relocate the unprefixed_value assignement so that it actually holds t…

[bdgregg]

…he unprefixed_value instead a duplicate of value. See Issue #3.

[ojas-uls-dev]

associated comment should be moved as well. imo having the variables be value and prefixed_value is more readable than trying to decode which cases have value as prefixed (prefixed_value probably should be set to either None or the original value if prefix does not exist)

[ctgraham]

This is plausible, but Tyrica's comment is ambiguous, and based on face-to-face discussion, I think the approach is wrong.

There are three fields which are mapped with prefixes here:
https://github.com/ulsdevteam/islandora_metadata/blob/f27a6843d206bf3a9644abc414a9081ed60a3a32/Ingest_Scripts/Utility_Files/manifest_to_i2_field_mapping.csv

• field_model
• field_resource_type
• field_depositor

Your debugging with Carolyn suggests that the expected merge operation is to have the output value unprefixed for fields other than field_linked_agent. This suggests that "Prevent duplicates with and without prefix" means "do not add a prefixed value if the same value exists unprefixed". Thus "image" + "resource_types:image" = "image". The proposed code does not cover that deduplication.

Actually, it does cover "image" + "resource_types:image" = "image"; what it doesn't cover is "resource_types:image" + "image" = "image". So, it might work, depending on the input. But it would be brittle, depending on the input.

[bdgregg]

@ctgraham, This is the suggested code replacement to handle duplicates with or without a prefix follows resulting with the without prefix being the preferred result. This seems to be a bit of a lift for the function and those functions calling it (which is probably why it wasn't suggested earlier). Does this feel like what you were trying to indicate?

Existing code:

    if prefix:
        value = f'{prefix}{value}'

    values = record.get(field, [])

    if field == 'field_linked_agent':
        # Prevent duplicates with prefix (names may repeat with different roles)
        if value not in values:
            values.append(value)
    else:
        # Prevent duplicates with and without prefix
        unprefixed_value = value
        if (
            value not in values
            and unprefixed_value not in values
        ):
            values.append(value)

    record[field] = values

    return value

Replacement code:

    unprefixed_value = value

    if prefix:
        value = f'{prefix}{value}'

    values = record.get(field, [])

    if field == 'field_linked_agent':
        # Prevent duplicates with prefix (names may repeat with different roles)
        if value not in values:
            values.append(value)
    else:
        # Prevent duplicates with and without prefix; when both a prefixed
        # and unprefixed form of the same term appear, keep the unprefixed
        # form regardless of which one arrived first.
        bare_map = (
            seen_bare_values.setdefault(field, {})
            if seen_bare_values is not None
            else None
        )

        if bare_map is None:
            if value not in values and unprefixed_value not in values:
                values.append(value)
        else:
            stored = bare_map.get(unprefixed_value)

            if stored is None:
                if value not in values:
                    values.append(value)
                bare_map[unprefixed_value] = value
            elif stored != unprefixed_value and value == unprefixed_value:
                # A prefixed variant is already stored; the unprefixed form
                # just arrived, so replace the prefixed entry with it.
                try:
                    idx = values.index(stored)
                    values[idx] = unprefixed_value
                except ValueError:
                    if unprefixed_value not in values:
                        values.append(unprefixed_value)
                bare_map[unprefixed_value] = unprefixed_value
            # else: duplicate of what's already stored; do nothing

    record[field] = values

    return value

The function parameters would change as well...

def add_value(
    result: ProcessingResult,
    record: dict,
    csv_field: str | None,
    field: str | None,
    value: str,
    prefix: str | None = None,
    seen_bare_values: dict[str, dict[str, str]] | None = None,
) -> str | None:
    """Add a processed value to a record field.

    Args:
        result: Runtime processing result.
        record: Record dictionary being updated.
        csv_field: Source CSV field.
        field: Target machine field.
        value: Raw value to add.
        prefix: Optional value prefix.
        seen_bare_values: Optional per-record tracking dict, mapping each
            field to a dict of bare (unprefixed) values already stored and
            the exact string currently holding that value in the record.
            Used to ensure that when the same term arrives both prefixed and
            unprefixed, the unprefixed form is what's kept.

    Returns:
        Processed value added to the record, or None.
    """

As well as those functions that need updated (add_value, add_title, process_title, process_model, process_record, process_model).
All five edits are in make_ingest_sheet.py:

• add_value — new parameter, rewritten dedup block
• add_title — new parameter, forwards it to both add_value calls
• process_title — new parameter, forwards it to add_title
• process_model — new parameter, forwards it to both add_value calls
• process_record — creates seen_bare_values once per record, passes it to the id call, the process_model call, the main add_value call inside the loop, and the final process_title call