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

Transactions cleanup #101

Merged
merged 3 commits into from
Sep 13, 2023
Merged

Transactions cleanup #101

merged 3 commits into from
Sep 13, 2023

Conversation

dogversioning
Copy link
Contributor

@dogversioning dogversioning commented Sep 13, 2023

This PR makes the following changes:

  • Removes template study uploads, just moving the file to archives (Discard data uploaded from template study #100)
  • Fixes one instance of a different key name for last_update causing multiple last update keys in transaction data
  • fixes spelling of transaction_format_version
  • Provides a migration script to apply these changes in different environments

scripts/migrations/migration.000.migrate_versioning.py Outdated Show resolved Hide resolved

# trimming erroneous keys
for site in transactions:
if site in EXPECTED_UPLOADERS:
Copy link
Contributor

Choose a reason for hiding this comment

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

Honest question: why this line? Why not migrate unexpected uploaders? And/or, maybe flag / warn about unexpected site uploads - that seems like a big surprise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so that's related to #101 (comment) - it is unexpected and it has happened, I think I IDed the root cause, and I'm trying to clean them up since we can't tie them back to a specific place and they make no sense in the aggregation tool i'm working on.

scripts/migrations/migration.001.transaction_cleanup.py Outdated Show resolved Hide resolved
tests/utils.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

general testing comments:

  • I would have expected to see some test changes for the last_upload key change - that code path must not be tested right now?
  • Probably worth adding a test for the new template archiving

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah fair point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - a true headache, but i've at least got a check for this case (and this was wrong to begin with, not just in terms of typos but in terms of which element is was pointed at).

I'm putting coverage on my short list for this repo.

Copy link
Contributor

@mikix mikix left a comment

Choose a reason for hiding this comment

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

I forgot to approve - I think my only real suggestion was the "needs more tests" one anyway and looks like you've added some

@dogversioning dogversioning merged commit e2894cc into main Sep 13, 2023
2 checks passed
@dogversioning dogversioning deleted the mg/upload_cleanup branch February 13, 2024 20:31
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.

2 participants