Support transactions in models #922
Conversation
There was a problem hiding this comment.
This is great! Even though you've summarized the PR down to "duplicating services and updating imports", I think a bit more has gone into it than just that! I took note of all the JSDoc updates, restructuring files in some instances to better follow the structure of the <model>Service.js files and a bit of intentionality and care with doing away with older parameter structures.
Excited for when we can get rid of services.deprecated. We've needed this approach for a while!
My inline comments only reflect minor cleanup needs.
There was a problem hiding this comment.
Hey Thanks for this work!
My main focus of this review is making sure that there are no missing or failing tests for the new object oriented structure of the API. As such, I found a few functions that may be missing tests:
AtService.js
1. removeAtVersionById
2. getUniqueAtVersionsForReport
BrowserService.js
1. removeBrowserVersionByQuery
2. findOrCreateBrowserVersion
CollectionJobService.js
1. scheduleCollectionJob automation/graphql tests only
2. restartCollectionJob automation/graphql tests only
3. cancelCollectionJob automation/graphql tests only
4. retryCanceledCollections graphql tests only
TestPlanReportService.js
1. updateTestPlanReportById
2. removeTestPlanReportById
UserService.js
1. getUserRoles
2. getUserAts
3. bulkGetOrReplaceUserAts
4. deleteUserFromRole
TestsService.js
1. getTests
TestResultWriteService.js
1. findOrCreateTestResult graphql / test plan run tests only
TestResultReadService.js
1. getTestResults
2. getFinalizedTestResults
TestPlanService.js
1. removeTestPlanById
TestPlanRunService.js
1. removeTestPlanRunResultsByQuery
2. getTestResultsUsingAtVersion
Also the parameter (t) that you are using in the new API is not very descriptive. It seems like the t represents transaction. Maybe we can make this param more descriptive in a future PR.
|
Hi reviewers, changes since the last review are here: https://github.com/w3c/aria-at-app/pull/922/files/a904404adc1ed8da85d47fe604f233898dc0deb6..73b597c8400f1f3d19f28f49056a57914563badd There are a lot of changes so I recommend taking a second look. @Paul-Clue, thanks for listing out those missing tests, while implementing them I actually found a bug and a couple of mislabeled fields so that was a great note. I didn't implement tests which were already covered elsewhere, but I made sure that all the methods which are universally tested in other services were tested. About the One other thing is that, as I worked on the follow-up PR to this one, I did find a couple more mislabeled fields. I've incorporated those changes into this PR. |
| * Gets one CollectionJob and optionally updates it, or creates it if it doesn't exist. | ||
| * Gets one CollectionJob and optionally updates it, or creates it if it doesn'transaction exist. |
There was a problem hiding this comment.
A typo was created here.
There was a problem hiding this comment.
Done, nice catch finding this in the voluminous changes 😅
| * Gets one TestPlanReport, or creates it if it doesn't exist, and then optionally updates it. Supports nested / associated values. | ||
| * Gets one TestPlanReport, or creates it if it doesn'transaction exist, and then optionally updates it. Supports nested / associated values. |
| // shouldn't have duplicate entries for a tester unless it's automated | ||
| // shouldn'transaction have duplicate entries for a tester unless it's automated |
Apologies for the ginormous PR, believe it or not this is only half of the larger effort to implement transaction support.
The file change count is high but most of the changed files only have one line in their imports changed. Also the lines changed is inflated since there's a lot of duplicated code.
Here is a summary of the changes:
These changes allow the entire system to keep working without changes. This is the strategy I've used to break this larger effort into two PRs.