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

feat: Track vendor approvals across version updates through result copy process #1267

Merged
merged 10 commits into from
Dec 5, 2024

Conversation

howard-e
Copy link
Contributor

@howard-e howard-e commented Nov 4, 2024

Address #1232

@howard-e howard-e changed the title Track vendor approvals across versions updates through result copy process Track vendor approvals across version updates through result copy process Nov 4, 2024
@howard-e howard-e changed the title Track vendor approvals across version updates through result copy process feat: Track vendor approvals across version updates through result copy process Nov 5, 2024
@howard-e howard-e marked this pull request as draft November 5, 2024 18:55
@howard-e howard-e requested review from stalgiag and removed request for stalgiag December 5, 2024 03:55
@howard-e
Copy link
Contributor Author

howard-e commented Dec 5, 2024

@stalgiag this is ready again for review after updating the tests

Copy link
Contributor

@stalgiag stalgiag left a comment

Choose a reason for hiding this comment

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

This is great. Thanks so much for the attention to detail in updating async and comparison code where you encountered it, adding in some utility testing methods, and writing an integration test that covers this feature. It was challenging to recreate the behavior manually but I managed with your instructions. Everything works as expected and the vendor approval status was carried over to the newer candidate version about promoting it. Great work!

if (isLastTest) finishButtonRef.current.focus();
// Prevent a plan with only 1 test from immediately pushing the focus to the
// submit button
if (isLastTest && tests?.length !== 1) finishButtonRef.current.focus();
Copy link
Contributor

Choose a reason for hiding this comment

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

Great job addressing this edge case :-)

)}'::jsonb) else element end)
from jsonb_array_elements(tests) as element )
where id = ${id}
and tests @> '[{"id": "${testId}"}]';
Copy link
Contributor

Choose a reason for hiding this comment

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

There is some interesting JSONB work I've never seen before, especially the containment operator. No notes, just wanted to remark.

// Update TestPlanVersion.tests to include the viewers from the old
// TestPlanVersion.tests
// TODO: Move viewers to TestPlanReport; more appropriate and
// understandable database structure
Copy link
Contributor

Choose a reason for hiding this comment

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

Heavy +1 on this TODO

).browser.id === '3'
).toBe(true);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Very impressed that you managed to reproduce this case. These tests are very helpful!

@stalgiag stalgiag merged commit 90807df into development Dec 5, 2024
2 checks passed
@stalgiag stalgiag deleted the track-vendor-approvals-across-versions branch December 5, 2024 21:17
howard-e added a commit that referenced this pull request Dec 10, 2024
Create December 10, 2024 Release

Includes the following changes:
* #1279, which addresses w3c/aria-at#1163
* #1267, which addresses #1232
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