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

Clean up incorrect database values in production #987

Merged
merged 3 commits into from
Mar 28, 2024
Merged

Conversation

howard-e
Copy link
Contributor

@howard-e howard-e commented Mar 26, 2024

Found 4 issues of varying importance while making changes to support #986:

  1. There were unexpected V23.08.23 versions included for all 35 test plans which seemed to have been created during the transition to the v2 test format and based on this commit. The hashTests function wasn't properly implemented at that time and I can only assume the imports being included were missed. These instances are all still in RD, and so can be removed without issue.
  2. As a result of no.1, there were 8 valid instances of test plan versions which were unnecessarily deprecated so we can revert that. The test plans are checkbox-tri-state, link-css, link-img-alt, main, meter, radiogroup-roving-tabindex, slider-multithumb, switch.
  3. The current aria-at.w3.org' toggle-button page has an additional version in DRAFT phase which shouldn't be possible. That is V23.12.13 should be shown as being deprecated by V23.12.14. I can only guess as to why this is but I assume this also stemmed from unexpected handling of data during the v2 test format update changes.
  4. Radio Group Example Using aria-activedescendant Test Plan V24.03.13 currently shows assertion phrases which should be using replaced by intended token strings as the generic string. ie Test 1 > JAWS > Tab (virtual cursor)'s last assertion phrase is saying "switch from reading mode to interaction mode" instead of "switch from virtual cursor active to PC cursor active" and the same is true for additional JAWS and NVDA assertions where this tokenization should have happened like on this aria-at preview page. That's because the PR which would support this happening during the import (and only during the import) has not yet been included in production and so it warrants an update happening in place.

This PR addresses the above points.

Copy link
Contributor

@gnarf gnarf left a comment

Choose a reason for hiding this comment

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

Having a migration that has no down step feels.... wrong.... Doing this in a migration feels... wrong....

I understand that a migration for this task might currently be the best option to patch/finesse the data on production, but we may want to consider finding a better one.

I am mostly worried about the inline comments and the possibility of deleting/updating random rows by id only out of an unknown database state.

@howard-e
Copy link
Contributor Author

howard-e commented Mar 27, 2024

This is data that would operate on the current state of the production database, that can never be returned to. The migration won't run again unless someone were to now delete the tracked migration from the production database.

The queries could be more specific to give that "confidence". But outside of a migration, it could be resolved to a one time patch done directly on the server, and noted as an incidence report since there isn't really a "down" to return here unless we're preserving all the current data for instances like this.

@howard-e
Copy link
Contributor Author

@gnarf pushed an update in e154971

Copy link
Contributor

@gnarf gnarf left a comment

Choose a reason for hiding this comment

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

If we are going to move ahead with the migration approach, the extra specificity fixes my main concerns - I assume the resulting information still checks out from a local test on your side (I'm not 100% sure where to verify it in the frontend on my side)

@gnarf
Copy link
Contributor

gnarf commented Mar 27, 2024

But outside of a migration, it could be resolved to a one time patch done directly on the server, and noted as an incidence report since there isn't really a "down" to return here unless we're preserving all the current data for instances like this.

I feel like unless it was seeded data, manual SQL updates could be peer-reviewed and executed on the live database (with appropriate backups) in a future where we try to ensure an up/down path for migrations is viable.

The main complaint I have seeing migrations like this comes from testing pull requests locally, if you can't safely up/down the migrations you can't safely switch between branches with any migration diffs. You are also a lot more restricted in ability to restore previous states to aid with bisecting a bug in the history. It could even be an issue on production. If there is some extreme issue with the next deployment for instance, we will be unable to revert the deploy to the previous version because of this migration alone (though I think an empty down() would maybe solve that for this one, now that it could be safely up()ed again without worry)

@howard-e
Copy link
Contributor Author

But outside of a migration, it could be resolved to a one time patch done directly on the server, and noted as an incidence report since there isn't really a "down" to return here unless we're preserving all the current data for instances like this.

I feel like unless it was seeded data, manual SQL updates could be peer-reviewed and executed on the live database (with appropriate backups) in a future where we try to ensure an up/down path for migrations is viable.

I'm all for that approach for the future. So PRs like this could be drafted or issues created to discuss the approach. With the result of the approval being a direct execution on the server and writing it up as a new incident report perhaps.

The main complaint I have seeing migrations like this comes from testing pull requests locally, if you can't safely up/down the migrations you can't safely switch between branches with any migration diffs. You are also a lot more restricted in ability to restore previous states to aid with bisecting a bug in the history. It could even be an issue on production. If there is some extreme issue with the next deployment for instance, we will be unable to revert the deploy to the previous version because of this migration alone (though I think an empty down() would maybe solve that for this one, now that it could be safely up()ed again without worry)

Makes sense. This just reminded me that sequelize explicitly requires the down() to support their cli command. At a minimum, I agree that an empty down should at least be included (which I've just added in ebc4825)

@howard-e howard-e merged commit a9df8e4 into main Mar 28, 2024
2 checks passed
@howard-e howard-e deleted the update-data branch March 28, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In production / Completed
Development

Successfully merging this pull request may close these issues.

2 participants