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

Add cojo to Schema Config #5388

Merged
merged 3 commits into from
Nov 12, 2024
Merged

Add cojo to Schema Config #5388

merged 3 commits into from
Nov 12, 2024

Conversation

sharadsw
Copy link
Contributor

@sharadsw sharadsw commented Nov 7, 2024

Fixes #5386

The missing relationships were due to COG -> cojo and CO -> cojo not being present in Schema Config. There were some more missing fields like createdByAgent and timestampCreated but those are from migration issues. If we test this PR with a fresh db, those fields shouldn't be missing

⚠️ Note: This PR affects database migrations. See migration testing instructions.

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

Testing instructions

  • Go to schema config
  • Select COG, COGT, or CO as base table
  • Click on uniqueness rules
  • Add new rule
  • Scroll down to relationships
  • Verify there are no empty fields/relationships

@sharadsw sharadsw added the Migration Prs that contain migration label Nov 7, 2024
@sharadsw sharadsw added this to the 7.9.9 milestone Nov 7, 2024
@sharadsw sharadsw requested review from acwhite211, CarolineDenis and a team November 7, 2024 18:14
@CarolineDenis CarolineDenis modified the milestones: 7.9.9, 7.9.8 Nov 8, 2024
Copy link
Contributor

@alesan99 alesan99 left a comment

Choose a reason for hiding this comment

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

Testing instructions

  • Go to schema config
  • Select COG, COGT, or CO as base table
  • Click on uniqueness rules
  • Add new rule
  • Scroll down to relationships
  • Verify there are no empty fields/relationships

image

No more blank relationships on those tables 👍

Side note: applying migrations on a production db still resulted in a single blank option (for children I think). Trying the migrations on a fresh db fixed it, but resulted in two children relationships appearing in the schema config, but its not related to this PR.

Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

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

Testing instructions

  • Go to schema config
  • Select COG, COGT, or CO as base table
  • Click on uniqueness rules
  • Add new rule
  • Scroll down to relationships
  • Verify there are no empty fields/relationships

Tried on two dbs and both resulted in some of the relationships still being empty. I am willing to believe this is a db problem however if it is, then we need to figure out which dbs do work. Tried with kufish212 and kubirds20240606

Screenshot 2024-11-08 095702

image

@sharadsw
Copy link
Contributor Author

sharadsw commented Nov 8, 2024

@emenslin It does seem like a db issue. I ssh'd into the kufish container and it looks like the migration in this PR didn't get applied at all. kufish is 5 migrations behind the latest:

specify
 [X] 0001_initial
 [X] 0002_geo
 [X] 0003_cotype_picklist
 [X] 0004_stratigraphy_age
 [X] 0005_collectionobjectgroup_parentcojo
 [X] 0006_fix_tectonic_tree_fields
 [X] 0007_schema_config_update
 [ ] 0008_ageCitations_fix
 [ ] 0009_tectonic_ranks
 [ ] 0010_updateDelete_parentcojo
 [ ] 0011_cascading_tree_nodes
 [ ] 0012_add_cojo_to_schema_config

I get this error when I try to apply those migrations and so I think this db is corrupted:

django.db.utils.OperationalError: (1060, "Duplicate column name 'AbsoluteAgeCitationID'")

kubirds20240606 does have all migrations so it's strange that it has missing fields.

specify
 [X] 0001_initial
 [X] 0002_geo
 [X] 0003_cotype_picklist
 [X] 0004_stratigraphy_age
 [X] 0005_collectionobjectgroup_parentcojo
 [X] 0006_fix_tectonic_tree_fields
 [X] 0007_schema_config_update
 [X] 0008_ageCitations_fix
 [X] 0009_tectonic_ranks
 [X] 0010_updateDelete_parentcojo
 [X] 0011_cascading_tree_nodes
 [X] 0012_add_cojo_to_schema_config

I noticed the fields in Schema config on that db are capitalized instead of camelCased. My guess is that the initial dump of that db already had an older version of the geo migration applied to it. The geo migration was changed later to use camelCased fields but those changes never got applied and so some newer fields never got added (parentCojo, createdByAgent and modifiedByAgent). I have manually reapplied all migrations to kubirds20240606 so it should work fine now

@sharadsw
Copy link
Contributor Author

sharadsw commented Nov 8, 2024

Pushed a fix for the duplicate children field. However, this would have to be tested by creating another copy of a db as it changes an old migration

combs-a
combs-a previously requested changes Nov 8, 2024
Copy link
Collaborator

@combs-a combs-a left a comment

Choose a reason for hiding this comment

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

image

Still blank for me too, followed the migration testing instructions as well. This was done with a clone of ChadronTest. Might be a database problem, but it's a bit concerning that 3 DBs that have been tried had migration issues.

@sharadsw
Copy link
Contributor Author

sharadsw commented Nov 8, 2024

Looks like #5384 has made the tectonic rank migration irreversible since the migration tries to delete the root rank when reversing, which is no longer allowed. I'll edit that migration and reapply migrations for the dbs mentioned in this PR.

@sharadsw
Copy link
Contributor Author

@combs-a Created a new db ChadronTest_fix where migration issues should be fixed

@pashiav pashiav requested a review from a team November 12, 2024 16:30
Copy link
Contributor

@pashiav pashiav left a comment

Choose a reason for hiding this comment

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

Testing instructions

  • Go to schema config
  • Select COG, COGT, or CO as base table
  • Click on uniqueness rules
  • Add new rule
  • Scroll down to relationships
  • Verify there are no empty fields/relationships

Empty field on COG. COGT, and CO do not have any empty fields!

https://cuic22024-issue-5386.test.specifysystems.org/specify/overlay/configure/uniqueness/collectionobjectgroup
Screenshot 2024-11-12 at 10 32 38 AM

@sharadsw
Copy link
Contributor Author

@pashiav I have reapplied all migrations on that db, should be fixed now

@pashiav pashiav requested a review from a team November 12, 2024 17:40
@sharadsw
Copy link
Contributor Author

sharadsw commented Nov 12, 2024

If a field other than COG -> cojo is missing, it is probably due to migration issues. Specifically, the geo migration needs to be reapplied to add newer COG/COJO/CO fields to Schema Config

DBs tested on this PR:
kufish212: corrupt db, cannot reverse migrations ❌
kubirds20240606: manually reapplied all migrations ✅
ChadronTest_fix: tectonic ranks migration cannot be reversed on this db specifically (and so COG -> parentCojo is missing) ❌
cuic22024: manually reapplied all migrations ✅

@sharadsw sharadsw dismissed stale reviews from combs-a and emenslin November 12, 2024 19:21

migration issues, info here: #5388 (comment)

@sharadsw sharadsw merged commit 1a84216 into production Nov 12, 2024
12 checks passed
@sharadsw sharadsw deleted the issue-5386 branch November 12, 2024 19:29
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: Done
Development

Successfully merging this pull request may close these issues.

Some relationships show as empty in uniqueness rules for geo tables
6 participants