Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing aggregation separators, add patches Django app #6115

Open
wants to merge 5 commits into
base: production
Choose a base branch
from

Conversation

grantfitzsimmons
Copy link
Member

@grantfitzsimmons grantfitzsimmons commented Jan 17, 2025

Warning

This PR affects database migrations. See migration testing instructions.

Fixes #5154, but retroactively, since this has already led to a lot of issues since the bug was introduced, and now its been insidiously spreading since (reported by numerous users, just solved another case today so I figured I should solve all of them at once).

This also creates a patches Django app that we can add to in the future when needing to do something like this.

It runs this on all databases:

UPDATE spappresourcedata spard
SET spard.data = REPLACE(spard.data, 'separator=""', 'separator="; "')
WHERE spard.SpAppResourceID IN (
    SELECT spar.SpAppResourceID
    FROM spappresource spar
    WHERE spar.Name = 'DataObjFormatters'
);

You cannot set the separator to nothing (""), so we can be very confident this solves cases where the default was lost and does not break cases where it is not desired.

Checklist

  • Self-review the PR after opening it to make sure the changes look good and
    self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone
  • Add relevant documentation (Tester - Dev)
  • Add a reverse migration if a migration is present in the PR

Testing instructions

Before using this branch, switch to v7.9.6.2 and edit the separator for any table aggregation.
Once you do, all default separators for table aggregations will be set to "" rather than "; ". This leads to very undesirable results where all Preparations or Collectors are aggregated together into a single string 😵

  • Verify that all separators are cleared in v7.9.6.2 in a DataObjFormatters app resource

     <aggregators>
     	<aggregator name="AccessionAgent" title="AccessionAgent" class="edu.ku.brc.specify.datamodel.AccessionAgent" default="true" separator="" ending="" format="AccessionAgent"/>
     	<aggregator name="AccessionAttachment" title="AccessionAttachment" class="edu.ku.brc.specify.datamodel.AccessionAttachment" default="true" separator="" ending="" format="identity" orderfieldname="ordinal"/>
     	<aggregator name="AccessionAuthorization" title="AccessionAuthorization" class="edu.ku.brc.specify.datamodel.AccessionAuthorization" default="true" separator="" ending="" format="AccessionAuthorization"/>
  • Pull this PR and run the migration

  • Verify that all empty separators have been restored to default

     	<aggregators>
     		<aggregator name="AccessionAgent" title="AccessionAgent" class="edu.ku.brc.specify.datamodel.AccessionAgent" default="true" separator="; " ending="" format="AccessionAgent"/>
     		<aggregator name="AccessionAttachment" title="AccessionAttachment" class="edu.ku.brc.specify.datamodel.AccessionAttachment" default="true" separator="; " ending="" format="identity" orderfieldname="ordinal"/>
     		<aggregator name="AccessionAuthorization" title="AccessionAuthorization" class="edu.ku.brc.specify.datamodel.AccessionAuthorization" default="true" separator="; " ending="" format="AccessionAuthorization"/>
     		<aggregator name="Address" title="Address" class="edu.ku.brc.specify.datamodel.Address" default="true" separator="; " ending="" format="Address"/>
     		<aggregator name="AgentAttachment" title="AgentAttachment" class="edu.ku.brc.specify.datamodel.AgentAttachment" default="true" separator="; " ending="" format="identity" orderfieldname="ordinal"/>

@grantfitzsimmons grantfitzsimmons changed the title Create patches Django app Fix missing aggregation separators, add patches Django app Jan 17, 2025
@grantfitzsimmons grantfitzsimmons added this to the 7.9.11.1 milestone Jan 17, 2025
@grantfitzsimmons grantfitzsimmons marked this pull request as ready for review January 17, 2025 19:03
@grantfitzsimmons grantfitzsimmons added the Migration Prs that contain migration label Jan 17, 2025
@grantfitzsimmons grantfitzsimmons requested review from a team and melton-jason January 17, 2025 20:25
Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@lexiclevenger lexiclevenger left a comment

Choose a reason for hiding this comment

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

  • Verify that all separators are cleared in v7.9.6.2 in a DataObjFormatters app resource
  • Pull this PR and run the migration
  • Verify that all empty separators have been restored to default

Looks good! All empty separators are restored.

Before:
Screenshot 2025-01-21 at 12 14 08 PM

After:
Screenshot 2025-01-21 at 12 17 50 PM


This branch also makes aggregators appear in the visual editor menu that weren't previously hidden without enabling 'Show All Tables' (mostly Attachment tables). I think all of these tables have default aggregators, so it's probably not a problem unless they really need to be hidden.

Before:
Screenshot 2025-01-21 at 12 24 45 PM

After:
Screenshot 2025-01-21 at 12 57 32 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Migration Prs that contain migration
Projects
Status: 📋Back Log
3 participants