Bug Fix for Transaction Read/Write Error #401
Conversation
dowhep
requested review from
sumeet-bansal,
shravanhariharan2 and
nik-dange
as code owners
|
Thanks for contributing! |
There was a problem hiding this comment.
I haven't found a fix for your testing issue yet, but an additional consideration I had was to find a way to send concurrent requests in a test case to ensure this feature works. Perhaps we can try to explore Promise.all or a similar construct–can discuss this more at our next dev meeting
| @@ -457,7 +457,6 @@ async function seed(): Promise<void> { | |||
| }); | |||
| const MERCH_ITEM_1_PHOTO_1 = MerchFactory.fakePhoto({ | |||
| merchItem: MERCH_ITEM_1, | |||
There was a problem hiding this comment.
Nit: maybe add these uploadedPhoto links back in for seeding data purposes now that you're done testing on the local frontend
There was a problem hiding this comment.
To be honest, the link before was just a dummy link, and since I added the default dummy link to the MerchFactory, I think it is no longer necessary to have it here. Do you think it is better to declare it explicitly instead?
There was a problem hiding this comment.
As long as there's no issues, I think it's fine to have it in the MerchFactory since it's a generic property of all merch item photos. If we do this though, maybe we should consider abstracting away any other common properties for seed data that we create. not high priority but something to keep in mind
tests/data/EventFactory.ts
Outdated
| @@ -37,10 +37,10 @@ export class EventFactory { | |||
| requiresStaff: FactoryUtils.getRandomBoolean(), | |||
| staffPointBonus: EventFactory.randomPointValue(), | |||
| committee: 'ACM', | |||
| cover: faker.internet.url(), | |||
| cover: `http://i.imgur.com/${faker.random.alphaNumeric(10)}.jpeg`, | |||
There was a problem hiding this comment.
For the sake of consistency and cleanliness, do you want to change all links to this format, or revert back to the old format? It's not a huge deal but it might be nice to keep our seeding data friendly to read, unless this is better for future local frontend testing (which likely won't rlly happen since staging will get ported to V2)
There was a problem hiding this comment.
I think this is worth discussing, but at the very least I should refactor this to a clean function call.
| @@ -15,6 +16,9 @@ import { LeaderboardRepository } from './LeaderboardRepository'; | |||
| import { ResumeRepository } from './ResumeRepository'; | |||
| import { UserSocialMediaRepository } from './UserSocialMediaRepository'; | |||
|
|
|||
| const HINT_FOR_RETRIABLE_TRANSACTIONS : string = 'The transaction might succeed if retried.'; | |||
| const AMOUNT_OF_RETRIES : number = 10; | |||
There was a problem hiding this comment.
are we consistent with declaring numbers like this at the top of the file or do we generally do this in the Config? small nit either way but for the sake of consistency
Info
Closes [#400]. (If there is no issue for this pull request yet, please create one or
delete this line if the pull request is for a very minor tweak).
Description
What changes did you make? List all distinct problems that this PR addresses. Explain any relevant
motivation or context.
I added the asynchronous retry for repository Read/Write to greatly reduce the chance of the error showing up.
Changes
Fill in later
Type of Change
expected)
workflows, linting, etc.)
If you've selected Patch, Minor, or Major as your change type, make sure to bump the version before merging in
package.json!Testing
I have tested that my changes fully resolve the linked issue ...
Checklist
package.jsonfile.Screenshots
Please include a screenshot of your Postman testing passing successfully.