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

Enforce foreign keys for mobility translations #655

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Splines
Copy link
Member

@Splines Splines commented Jun 27, 2024

In #609, we migrated from globalize to mobility, a "Pluggable Ruby translation framework". We migrated according to their guide here. However, we didn't check our own migrations file. Now we noticed that three very old migrations cannot be run anymore because the create_translation_table! method used in them was provided by globalize and is no longer part of mobility, see here.

For testing purposes, in #654, we deleted those non-working migrations and completely setup the database again by deleting it entirely (also its schema), then running through all migrations. After that, we used the generator provided by mobility to generate the respective translation tables again:

rails generate mobility:translations subject name:text
rails generate mobility:translations program name:text
rails generate mobility:translations division name:text

The resulting schema.rb can now be diffed with what we had before, see this diff. We made the following observations:

  • Foreign key constraints were added, which is probably due to Table-backend migration generator creates wrong migrations shioyama/mobility#281 and the respective PR. In this current PR you're viewing, we add those foreign keys manually in a new migration.
  • Indices were merged together into one line. We don't do anything here as this is not a semantic change.
  • ⚠ The field t.text "name" was added to the table subjects, programs and divisions. However, this field already exists in the respective translation tables, so we don't know why we would need it in the associated table as well. We don't change this in this PR as mobility already worked in the last few months without this field. If we have problems with mobility in the future, this might be a possible reason.
    Note that in the translation tables the field t.text was just moved, so it seems like it was added there in the diff as well. It's different for the "non-translation tables" subjects, programs or divisions where the field was really newly added (there's no corresponding red "delete" line for t.text "name" in those tables.

I've added a Migration label to GitHub such that it's easier to find out which PRs were responsible for database migrations ;)

Deployment to experimental

I've just deployed this commit to experimental and the server was not reachable afterwards. In the logs, I couldn't find any root cause, nor anything that pointed to the change I made here. I now reset experimental to a commit prior to db9d1f1, build it, then set it to db9d1f1 again and see if it works.

Edit: apparently it also doesn't work with the commit before this PR:

Edit: found the mistake, see this comment. TL;DR: the deployment issue was not related to this PR.

@Splines Splines self-assigned this Jun 27, 2024
@Splines Splines changed the title Add foreign keys for mobility translations Enforce foreign keys for mobility translations Jun 27, 2024
@Splines Splines added the migration Database migrations label Jun 27, 2024
Copy link

codecov bot commented Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.64%. Comparing base (ed867fb) to head (db9d1f1).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #655   +/-   ##
=======================================
  Coverage   66.64%   66.64%           
=======================================
  Files         309      309           
  Lines        9386     9386           
=======================================
  Hits         6255     6255           
  Misses       3131     3131           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Splines Splines marked this pull request as ready for review June 27, 2024 22:49
@Splines Splines requested a review from fosterfarrell9 June 28, 2024 14:37
@fosterfarrell9
Copy link
Collaborator

I still need some convincing as to why we need these foreign keys. If they were that important, wouldn't they be mentioned in the migration guide from globalize to mobility? Also, I think it would be good to find out what the issue with staging is and to verify that it is independent of the PR under review.

@Splines
Copy link
Member Author

Splines commented Jul 1, 2024

Also, I think it would be good to find out what the issue with staging is and to verify that it is independent of the PR under review.

The issue with mampf-experimental was due to #609 where we activated the option config.require_master_key. After having filled the field RAILS_MAILS_KEY for the respective docker.env, the staging worked again for commit 0c841de on dev. I've just rebuilt it with the latest commit from this PR (db9d1f1) and it also ran through without any problems.


I still need some convincing as to why we need these foreign keys. If they were that important, wouldn't they be mentioned in the migration guide from globalize to mobility?

You're right that this is unclear. I've opened an issue at mobility, maybe they can help: shioyama/mobility#641

@Splines
Copy link
Member Author

Splines commented Jul 2, 2024

I will convert this back to a draft until we get a response here: shioyama/mobility#641. This is not a blocker as mobility is working just fine in production; we just want to make sure that we didn't miss anything in the migration.

@Splines Splines marked this pull request as draft July 2, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration Database migrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants