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

Ensure Each TestQueueRow Uses a Unique ID-ref. #370

Merged
merged 2 commits into from Feb 1, 2022

Conversation

jkva
Copy link
Contributor

@jkva jkva commented Dec 6, 2021

This change ensures each id generated for a Test Queue Row combines its test plan ID and tester name. This addresses a problem where two test rows assigned to the same tester generate (elements with) duplicate IDs.

@jkva jkva requested a review from jscholes December 6, 2021 10:29
@jscholes
Copy link
Contributor

jscholes commented Dec 6, 2021

@jkva I understand the intent behind this, but I'm not sure I understand the implementation/resulting mark-up. Each row in the test queue represents a test plan that can be assigned to multiple people, not just one.

@jkva
Copy link
Contributor Author

jkva commented Dec 6, 2021

@jscholes previously, you would get multiple elements with id assignee-[username]-completed, so in the case of multiple plans with the same assigned user, this would result in duplicate ids on the page.

@jscholes
Copy link
Contributor

jscholes commented Dec 6, 2021

@jkva I looked at the rendered mark-up in the actual app as it is currently deployed, and this makes more sense now. The use of the word "row" is what was tripping me up. You're talking about a div, representing a tester's completion status, inside a list item, in an ul. But that content itself is inside a table. So when you said "row", I thought you meant a row of the table.

However, now that I understand it, I want to confirm that the proposed change fully resolves the issue. The Test Queue will contain multiple instances of the same plan, across different browser/AT combinations. If the same tester is assigned to the same plan across multiple combos, will the IDs still be unique?

@jkva
Copy link
Contributor Author

jkva commented Dec 6, 2021

That's a good use case for me to double-check, thank you.

@jkva
Copy link
Contributor Author

jkva commented Dec 7, 2021

@jscholes I've pushed a commit including the Test Plan Run ID within the generated ID, further guaranteeing uniqueness.

Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

LGTM other than the optional change request.

currentUserTestPlanRun.id,
'assignee',
tester.username,
'completed'
Copy link
Contributor

@howard-e howard-e Jan 27, 2022

Choose a reason for hiding this comment

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

Optional nitpick to address: I understand the id's original structure was being preserved with -completed being appended. Taking another look again at its use, perhaps something like -completed-tests is clearer for the id's purpose, but removing -completed altogether also works well and is helpful to reduce some of the id's verbosity.

@evmiguel evmiguel merged commit 4bbdc17 into main Feb 1, 2022
@evmiguel evmiguel deleted the generate-unique-test-row-id branch February 1, 2022 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants