fix(reviewRequestService): upsert in case of concurrent api calls

[kishore03109]

Problem

Currently, when calling the the route /viewed twice, this might lead to errors thrown in the backend. While the logic is sane, the reason why the errors occur is because there is a race condition when two threads attempt to create at the same time.

Solution

Upsert rather than create. Note that the comments.spec.ts already needed these file changes to work.

Limitations

This not the ideal way to solve race conditions. The ideal method is to create a lock and then keep the function markAllReviewRequestsAsViewed atomic. However, at this point of time, that might be over-engineering here as the only impact that could happen with unlikely concurrent calls to markAllReviewRequestsAsViewed is the dashboard does not turn blue to mark it as unread.

Following Chin's suggestion of using transactions, this limitation is no longer valid.

Tests

e2e for notifications pass (failed without these changes)

Misc
There is another issue of our /viewed being called twice, which is why this bug was noticed in the e2e testing in the first place. While that might incur separate effort to investigate, this PR intends to quick fix this regardless as this can still occur say when a user has 2 tabs open and refreshes them simultaneously.

[seaerchin]

hmm, maybe the correct thing to do would be to use a transaction here. wdyt?

[kishore03109]

ya you're right

[alexanderleegs]

lgtm - let's still file an issue for the root cause of the issue though

[seaerchin]

just realised that this is cos of repeated attempts to create - in this case, the transaction might not help at all actually because we'd still try to create the repeated entry.

maybe we should just use _.unique(requestIdsToMarkAsViewed) to prevent dups and accept hte race condition of having some views being outdated (actually seen but presented as not), as it's relatively minor.

[kishore03109]

in this case, the transaction might not help at all actually because we'd still try to create the repeated entry.

hmm i am confused here, why would it try to create a repeated entry ya? there is still a race condition here, but in this case we are "safely" failing here right due to a rollback? so rather than making our server return 500, we are increasing the stability of our be here no?

maybe we should just use _.unique(requestIdsToMarkAsViewed) to prevent dups and accept hte race condition of having some views being outdated (actually seen but presented as not), as it's relatively minor.

hmm wait ah this is due to duplicate api calls leh, how would this help here ah?

[seaerchin]

i think the error stems from an attempt to create when there's an existing entry + a unique constraint. this is not due to concurrency issues and you can replicate this just by having the requestIds to be identical and do the creation sequentially.

because the error is due to duplicates in requestIds, the fix is to either remove the duplicates or allow the update but have it do nth.

[kishore03109]

wait we do have an initial call here right, in line 493?

 const requestsViewed = await this.reviewRequestView.findAll({
          where: {
            siteId: site.id,
            userId,
          },
          transaction,
        })

we get the list of requestsViews, diff it using the current open review requests, and iff there are active requests that have not been viewed yet, create a entry in the reviewRequestViews?

[seaerchin]

if you've tried that this solution works on local, then please merge it. i might have a misconception, so it shouldn't block the PR

[seaerchin]

should write a simple spec to prevent regression

[kishore03109]

a1c8782

[seaerchin]

actually because the transaction here is mocked, it mihgt not be very useful. we might wish to write an integration spec and verify that it fails initially (without the fix) and passes later on with the fix

[kishore03109]

^ ok, i tackle this in a separate pr cuz at least the changes should be merged in by tmr's release uh