-
Notifications
You must be signed in to change notification settings - Fork 0
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
Transactions cleanup #101
Conversation
d6607d6
to
ae2f48b
Compare
|
||
# trimming erroneous keys | ||
for site in transactions: | ||
if site in EXPECTED_UPLOADERS: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
tests/utils.py
Outdated
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah fair point
There was a problem hiding this comment.
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.
There was a problem hiding this 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
This PR makes the following changes:
template
study uploads, just moving the file to archives (Discard data uploaded from template study #100)last_update
causing multiple last update keys in transaction datatransaction_format_version