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

Update deduplicate migration #739

Merged
merged 5 commits into from
Aug 14, 2023
Merged

Update deduplicate migration #739

merged 5 commits into from
Aug 14, 2023

Conversation

howard-e
Copy link
Contributor

@howard-e howard-e commented Aug 7, 2023

Resolves #579.

This updates the de-duplication migration to check the batches of git commits, and only save the test plan versions which match up with a valid git sha, referenced through the GitHub API.

This will make it so ONLY known git commits (in the database at this time) are shown for respective Test Plans, which addresses #579. Note that this PR does not back-populate any commits that may have been missed.

NOTE
This causes data to be lost for the entire rows of Action Menu Button Example Using aria-activedescendant and Toggle Button when comparing against the production /reports page, because those reports were created with test plan versions that weren't valid and couldn't be easily updated by this migration.

Drafting to determine how the migration can avoid causing the data loss, or we may need to consider manually updating that data separately

@howard-e howard-e requested a review from alflennik August 7, 2023 20:33
@howard-e
Copy link
Contributor Author

howard-e commented Aug 7, 2023

Importing a commit entry for w3c/aria-at@0223400 with the import script, into TestPlanVersion before running the migration will allow the data to be preserved for Toggle Button. Haven't found the same for Action Menu Button Example Using aria-activedescendant as yet.

Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

I'll wait for you to mark this as ready to review but the direction looks good 👍

@@ -278,6 +282,215 @@ module.exports = {
);
};

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Beast mode starts here lol

@howard-e
Copy link
Contributor Author

howard-e commented Aug 14, 2023

@alflennik after making sure an entry exists for the Toggle Button version and the Action Menu Button Example Using aria-activedescendant, which the migrated reports will be linked with, this should work without any data loss.

@howard-e howard-e marked this pull request as ready for review August 14, 2023 14:36
@howard-e howard-e requested a review from alflennik August 14, 2023 14:41
Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

It's actually pretty magical to see that every single commit is now directly referring to the test plan it's paired with. This seems like a big win for the usability of the app. I did find one commit that seemed to be miscategorized, but when I looked into it I saw that it did indeed change the source code for two test plans, so the behavior was expected. I verified it working with a fresh database and with the clone of the production database. Thanks for getting this PR over the finish line, it's even more polished than I imagined possible when I implemented the first version of this.

@howard-e
Copy link
Contributor Author

howard-e commented Aug 14, 2023

@alflennik thanks for the review!

I also noticed something else which is that, with TestPlanVersion.tests[].viewers also being updated, it had to be ignored in the hashTests function or we could risk an import being duplicated again as the hash could be different if viewers existed for that row.

Using the following query on the latest production data, 5 rows were found before the migration and 2 rows after. 3 of those rows were meant to be correctly removed so we don't need to do anything additional here. But I mention it in case any future migrations or functions require moving of existing viewers on a TestPlanVersion to another:

select *
from "TestPlanVersion"
where exists ( select 1 from jsonb_array_elements(tests) as test where jsonb_array_length(test -> 'viewers') > 0 );

I also think in the future, we may want to have viewers being its own column but that's a separate discussion.

@howard-e howard-e merged commit a4ca04f into main Aug 14, 2023
@howard-e howard-e deleted the update-deduplication-migration branch August 14, 2023 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants