mampf icon indicating copy to clipboard operation
mampf copied to clipboard

Enforce foreign keys for mobility translations

Open Splines opened this issue 1 year ago • 4 comments

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 https://github.com/shioyama/mobility/issues/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 avatar Jun 27 '24 22:06 Splines

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 66.64%. Comparing base (ed867fb) to head (db9d1f1). Report is 70 commits behind head on dev-archive.

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jun 27 '24 22:06 codecov[bot]

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.

fosterfarrell9 avatar Jun 30 '24 10:06 fosterfarrell9

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 0c841de5b5500 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: https://github.com/shioyama/mobility/issues/641

Splines avatar Jul 01 '24 17:07 Splines

I will convert this back to a draft until we get a response here: https://github.com/shioyama/mobility/issues/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 avatar Jul 02 '24 19:07 Splines

Closing this, as it works for us right now and we got a responsive that we don't actually need this.

Splines avatar Jun 10 '25 18:06 Splines