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

[WIP] Change DraftItem date_created value to string type #2952

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lagoan
Copy link
Contributor

@lagoan lagoan commented Aug 22, 2022

Context

This is an initial migration to change the date_created value type from date to string. The strong_migrations gem complained about a simple type change so I followed the suggested process:

  1. Create a new column
  2. Write to both columns
  3. Backfill data from the old column to the new column
  4. Move reads from the old column to the new column
  5. Stop writing to the old column
  6. Drop the old column

At this point, it would be beneficial to get input on the migration process to see if we need this approach or stick to something simpler. Any feedback is greatly appreciated.

Related to #2931 , #2951

What's New

Initial migration changing type from date to string.

DraftItem model needs to accept dates in format 'YYYY-MM-DD',
'YYYY-MM', and 'YYYY'. The current model's behavior does not allow this
due to the date_created value type is date, which only accepts dates
with formats 'YYYY-MM-DD', 'YYYY-MM'. Furthermore the UI needs to change
in order to accept this change.

The date_crated type will change from date to string and we need to add
back and front end validations to handle partial dates.
@pengyin-shan
Copy link

Hi @lagoan sorry I don't know much about Jupiter...sounds like we want to change a column type from timestamp to string? Your plans sound good to me. I was thinking in the last step, instead of dropping the column, probably just rename it to some other name in case we need it again sometime. However, I totally trust your judgment because this way could create legacy data at the same time.

Also do you need me to approve this PR ASAP? or wait for others' feedback?

@lagoan
Copy link
Contributor Author

lagoan commented Aug 22, 2022

Thanks for your comment @pengyin-shan. That is right, I need to change the column type from date to string. I am conflicted about dropping the column. If we remove it the bad side would be problems with the migration and loosing some of that data, as well as the legacy data. The good side would be a cleaner database with no noise in the data. This is the outline suggested by the developers of the strong_migrations gem but it would help a lot if I can get more eyes on it just to make sure.

At this time I am just looking for feedback, there is a lot of more work to do before it can ger properly merged

@pengyin-shan
Copy link

Thanks for your comment @pengyin-shan. That is right, I need to change the column type from date to string. I am conflicted about dropping the column. If we remove it the bad side would be problems with the migration and loosing some of that data, as well as the legacy data. The good side would be a cleaner database with no noise in the data. This is the outline suggested by the developers of the strong_migrations gem but it would help a lot if I can get more eyes on it just to make sure.

At this time I am just looking for feedback, there is a lot of more work to do before it can ger properly merged

Sounds good!👍

@github-actions
Copy link

1 Error
🚫 Please include a CHANGELOG entry.
You can find it at CHANGELOG.md.
1 Warning
⚠️ There are code changes, but no corresponding tests. Please include tests if this PR introduces any modifications in behavior.

Generated by 🚫 Danger

@pgwillia
Copy link
Member

pgwillia commented Aug 25, 2022

My reading of the strong_migration suggestions is that ideally each of these steps should be in a separate deployment. Would it work or otherwise cause problems to have one app server run the migration while another is still reading the database? My reading of this migration is that the other app server would not apply the DraftItem.class_eval blocks the way this is written.

It is possible that following the strong_migrations practice is overkill for us and we should remove/ignore it?

https://ualbertalib-di.slack.com/archives/CSEHP0L0K/p1636567438054800
https://ualbertalib-di.slack.com/archives/CSEHP0L0K/p1637180734087300?thread_ts=1636567438.054800&cid=CSEHP0L0K
#2621 (comment)

@pgwillia
Copy link
Member

I'd recommend using edtf-ruby because it comprises a parser and an API implementation of the Extended Date/Time Format standard.

It was introduced for Digitization content where edtf was a requirement from the metadata team. You can see an example of a validator I wrote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants