Skip to content

Let the view know about the review status of the PR #1833

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

Merged
merged 8 commits into from
Aug 12, 2021

Conversation

RishabhKothaari
Copy link
Contributor

@RishabhKothaari RishabhKothaari commented May 26, 2020

Fixes #473
The PR model must find the current user's review status for the PR and send it to the view.
Let me know if it requires any further changes.

The PR model should send the review status of the PR to the view.
@RMacfarlane RMacfarlane self-assigned this May 26, 2020
Base automatically changed from master to main February 17, 2021 21:41
@kieferrm kieferrm assigned alexr00 and unassigned RMacfarlane Jun 7, 2021
@alexr00 alexr00 added this to the June 2021 milestone Jun 7, 2021
@alexr00 alexr00 modified the milestones: June 2021, July 2021 Jun 29, 2021
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

This PR is looking good! If the merge conflicts are resolved I will take another look.

@alexr00 alexr00 removed this from the July 2021 milestone Jul 16, 2021
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

Accidentally hit approve.

@RishabhKothaari
Copy link
Contributor Author

RishabhKothaari commented Jul 22, 2021

@alexr00 Thanks for reviewing it. I will resolve the conflicts. I hope I can get back quickly, it has been a year since I have worked on VSCPR

@RishabhKothaari RishabhKothaari requested a review from alexr00 July 22, 2021 01:56
@alexr00 alexr00 added this to the July 2021 milestone Jul 22, 2021
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Just one more question.

@@ -158,6 +158,7 @@ export class PRContext {
});
state.reviewers = reviewers;
state.events = [...state.events.filter(e => (isReviewEvent(e) ? e.state !== 'PENDING' : e)), review];
state.reviewState = review.state;
Copy link
Member

Choose a reason for hiding this comment

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

Is a check needed here to make sure that the review is from the current user?

Copy link
Contributor Author

@RishabhKothaari RishabhKothaari Jul 30, 2021

Choose a reason for hiding this comment

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

@alexr00 I did not see a need to check if the review is from the current user. As this refers to the review state of the PR for the current user. It is required to enable/disable the Approve button on the overview of the PR.
When the PR panel is shown it always refers to the review state from the current user for the PR based on the return from pullRequestOverview.ts#L153
When the reviewer makes changes to the PR review always refers to the current user's review on the PR.
Please let me know if I am missing anything here, I am happy to make changes/updates

@alexr00 alexr00 modified the milestones: July 2021, August 2021 Jul 28, 2021
@RishabhKothaari RishabhKothaari requested a review from alexr00 July 30, 2021 18:46
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@alexr00 alexr00 merged commit 7fbc64c into microsoft:main Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Approve button should be disabled when PR is alreay approved
3 participants