feat: fetch only latest open PR from DB

[harishv7]

Problem

Currently we are fetching all the PRs (Open + Closed) from our DB. Then we do a for-loop over every PR to get its meta, number of comments etc. from Github API.

This causes unnecessary calls to Github. In line with our effort to reduce Github API calls, and with the fact that there could be at most 1 open PR for a repo (so far), we can further streamline this.

Closes IS-171

Solution

We only retrieve the latest (newest by createdAt timestamp) PR that is in "OPEN" or "APPROVED" state.

Since we may have more than 1 Open PR in future, the changes allow for us to revert back to using findAll in a relatively simple way.

Breaking Changes

• Yes - this PR contains breaking changes
  • Details ...
• No - this PR is backwards compatible

Test Cases

Site dashboard

• should show only 1 open or approved review request

Workspace

• should show the review request alert when there's a review request open

site edit header (this is used widely)

• should disable the review request button when there's an open review request
  approved review redirect (used widely)

• should redirect when there's an approved review request
  more broadly, we want to test for this property (can't think of good test cases off the top of my head):

essentially, if there's an open/approved RR, we can't raise any more RR
verified by @kishore03109

[kishore03109]

[Nit] I would rather have this function to only resolve one promise with no comments rather than a map over the array + add a todo saying we can change this to findAll when we allow multiple PRs in the future since we are not certain about the future and this comment might end up being obsolete/redundant down the line if we never intend to do so.

[Tests] Tests are failing :( I am approving this on the assumption that making the tests pass does not require a change in the ReviewRequestService.ts. Please dismiss this review if that is not true.

[harishv7]

[Nit] I would rather have this function to only resolve one promise with no comments rather than a map over the array + add a todo saying we can change this to findAll when we allow multiple PRs in the future since we are not certain about the future and this comment might end up being obsolete/redundant down the line if we never intend to do so.

[Tests] Tests are failing :( I am approving this on the assumption that making the tests pass does not require a change in the ReviewRequestService.ts. Please dismiss this review if that is not true.

@kishore03109 Hmm, this PR is based on the assumption that we are tackling multiple open PRs in the near term roadmap. That's the reason the implementation was done as a "short-term" fix

But if you were to change it to a single promise, then the return type of the function will become Promise which is different. As such the changes will cascade to the caller's return type and the logic as well and cascade on the tests we have.

The reason behind the changes are that we want the simplest path to unroll back into using findAll to return multiple results.

[kishore03109]

Sanity check here: do we only want open reviews? Dont we also need Approved and/or closedones because they are open but have not been merged in yet?

[harishv7]

@kishore03109 During discussion we agreed to fetch the latest open PR, which I felt referred to the PRs in "open" status.

@seaerchin Do we need to displays Approved/Closed PRs which are not yet merged?

[seaerchin]

good catch @kishore03109! i searched the FE for this - we'd need to give both APPROVED | OPEN states. could i also trouble you (@harishv7) to rename this + the route to listValidReviewRequests for clarity?

[harishv7]

done with renaming and fetching for both approved + open states. As discussed,

There can only exist 1 active PR which is in either Open or Approved state. The new logic will fetch the most recently created PR DB entry which is in Open/Approved state

[kishore03109]

wait sanity check, the code here still filters by reviewStatus: ReviewRequestStatus.Open, and not by approved + open state, is this intended?

[harishv7]

@kishore03109 Looking at https://github.com/isomerpages/isomercms-backend/pull/769/files#diff-ec790b65dee9ec111ee3c30c98efd651b561a57b31c998d06b200db95e68255f, code is updated to

const request = await this.repository.findOne({
      where: {
        siteId: site.id,
        [Op.or]: [
          {
            reviewStatus: ReviewRequestStatus.Open,
          },
          { reviewStatus: ReviewRequestStatus.Approved },
        ],
      },

missing condition

[seaerchin]

Hey, just searched the FE for this endpoint and added test cases below! feel free to copy as-is to the PR description and/or add on!

Site dashboard

• should show only 1 open or approved review request

workspace

• should show the review request alert when there's a review request open

site edit header (this is used widely)

• should disable the review request button when there's an open review request

approved review redirect (used widely)

• should redirect when there's an approved review request

more broadly, we want to test for this property (can't think of good test cases off the top of my head):

1. if there's an open/approved RR, we can't raise any more RR

this might be out of scope at present, but we might wanna check if creation of RR is blocked on BE also