Namespace approval issues fix - #12502
Helen Gao (helen229) wants to merge 17 commits into
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds tracking of whether a namespace review has been requested at both API revision and review list levels, and updates UI logic to disable namespace review actions once approved or already requested. Key changes:
- Front-end: Added hasRequestedNamespaceReview flag to APIRevision model and integrated state handling in review page/options components.
- UI logic: Adjusted enable/disable conditions and button state handling for namespace review requests when a review is approved or already requested.
- Back-end: Added HasRequestedNamespaceReview persistence and expanded logging plus revision flag update during namespace review request flow.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dotnet/APIView/ClientSPA/src/app/_models/revision.ts | Added hasRequestedNamespaceReview property and default initialization. |
| src/dotnet/APIView/ClientSPA/src/app/_components/review-page/review-page.component.ts | Updates revision state after requesting namespace review. |
| src/dotnet/APIView/ClientSPA/src/app/_components/review-page-options/review-page-options.component.ts | Incorporates new namespace review state logic and button disabling for approved reviews. |
| src/dotnet/APIView/ClientSPA/src/app/_components/review-page-options/review-page-options.component.html | Disables request button when review is approved. |
| src/dotnet/APIView/APIViewWeb/Managers/ReviewManager.cs | Marks revision as having requested namespace review and adds extensive diagnostic logging. |
| src/dotnet/APIView/APIViewWeb/LeanModels/ReviewListModels.cs | Adds HasRequestedNamespaceReview property to review list model. |
|
Helen Gao (@helen229) please add a description about what you are fixing, that would help us a lot with the review! also, if you can include more details on the title of the pull request, it would be great :) |
There was a problem hiding this comment.
helen229 please add a description about what you are fixing, that would help us a lot with the review! also, if you can include more details on the title of the pull request, it would be great :)
Address all of these issues. These are minimum standards expected of all PRs.
Helen Gao (helen229)
changed the title
Alitzel Mendez (AlitzelMendez)
left a comment
Alitzel Mendez (AlitzelMendez)
left a comment
There was a problem hiding this comment.
This issue: #12215 is still present in this pull request.
As it is its own issue, you can either:
- Remove it from the description and create a separate pull request
- Fix this scenario in this pull request
I did a repro in Microsoft.SecurityInsights
I just approved all related reviews and then requested namespace approval. All the related reviews were already approved, but this wasn't checked, so the status is stuck.
| /** | ||
| * Check if all associated SDK language reviews are approved | ||
| */ | ||
| private checkAssociatedReviewsApprovalStatus() { |
There was a problem hiding this comment.
There are a couple of things that are unclear to me about this solution. So basically, everything is handled on the UI side, and there is no update on the server side?
If I request a review for this scenario, I will send an email to approve this, right? To the architects? And then all of them will be approved? I will change this button, but I will never update anything on their end—is that what is happening? On top of that, the database will never reflect that it was approved (for TypeSpec).
If this is the case, even though it works visually, I don’t think it’s the right solution because the database information is no longer reliable. We send an email for the architects to check when they don’t actually need to check anything, and we never update that it was indeed approved. This introduces some problems.
|
I think this PR is growing more than it should. Right now, this pull request is fixing four issues, and at this point, it’s hard to split. But in the future—and even if you can do something in this pull request—it would be great if we could handle one pull request per issue. When I review it now, I need to think about four different workflows, which makes it harder to review every iteration, smaller prs are better :) |
|
Helen Gao (@helen229) I was doing a final test, and I used: https://spa.apiviewuxtest.com/review/cbdb6106572b4c358de8e517ed4e383b?activeApiRevisionId=c4af63d2a56244a1af48ad769a867252 Requested
Everything looked good! But then I went to the emails and I saw a bunch more packages than the two that appeared on the requested tab: The first two are the ones that I approved, but the others are not approved. The "First Release Approval" button is still enabled, meaning they need approval. So I checked if they were actually related. According to https://spa.apiviewuxtest.com/review/cbdb6106572b4c358de8e517ed4e383b?activeApiRevisionId=c4af63d2a56244a1af48ad769a867252, the related pull request is Azure/azure-rest-api-specs/38509 which is also here: So everything points to the fact that this is not fully approved yet, that there are pending languages. But the namespace says that it is approved. What is happening here? Is this a bug introduced in this pull request? If that is the case, please add a test that reproduces this error, not just fixes it. We need a test - this is core functionality that should always be tested. If we break this with any code change, a test should let us know. We can't rely on manual testing, and it is time-consuming. Please update me if you have any problem reproducing this in UX, and if you have any issue reproducing it in a unit test, I really want a unit test for this scenario. |
|
|
||
| [Fact] | ||
| public async Task RequestNamespaceReview_WithUnauthorizedUser_ShouldThrowUnauthorizedException() | ||
| public async Task RequestNamespaceReview_WithUnauthorizedUser_ShouldStillCompleteRequest() |
There was a problem hiding this comment.
I don't understand this one, why are we supporting to not fail and actually do the request for someone unauthorized?
There was a problem hiding this comment.
Helen clarified that anyone that can access to apiview can request this, so please just delete these test, is misleading, people need authorization to even call the endpoint, I verified that, so please remove it
| } | ||
|
|
||
| [Fact] | ||
| public async Task RequestNamespaceReview_WithFeatureDisabled_ShouldStillProcessRequest() |
There was a problem hiding this comment.
if the feature is disable, why are we still processing the request?
I don't think this is something core for our code, we should delete this test
| } | ||
| catch (Exception ex) | ||
| { | ||
| _telemetryClient.TrackException(ex); |
There was a problem hiding this comment.
What happens if something fails with this line in place? This will make the whole request end successfully—the button changes to “Requested namespace, waiting”—but the part that failed won’t have the correct status and won’t be able to get triggered.
I don’t think this is common, which is why I didn’t mention it before, but I see catch blocks throughout the code. I want to understand what the plan is for this scenario. For example, if the database fails and you’re unable to save the record, how can I re-trigger the action? What feedback do we get if a related item wasn’t updated?
It’s not the biggest issue since it shouldn’t be common, but this could be blocking. People might question why, if “everything” was approved, it’s still not working—because instead of failing and stopping, we’re silently failing.
| } | ||
| catch (Exception ex) | ||
| { | ||
| _telemetryClient.TrackException(ex); |
There was a problem hiding this comment.
Again, this is a silent failure. In this case isn’t too bad—okay, they weren’t notified—but at least there’s a tab where they can find the related items or approve them if needed. However, let’s consider in the future whether we need to implement retries or other actions, rather than just catching a failure.
Alitzel Mendez (AlitzelMendez)
left a comment
Alitzel Mendez (AlitzelMendez)
left a comment
There was a problem hiding this comment.
I did this test: https://spa.apiviewuxtest.com/review/cbdb6106572b4c358de8e517ed4e383b?activeApiRevisionId=c4af63d2a56244a1af48ad769a867252
Everything is approved, doesn't appear as approve and it has an approve button
|
Most of the issues listed here have been fixed:
However, other things are still broken, and honestly, I can’t continue reviewing a pull request that touches so many areas without proper test coverage. I see there are tests in the code, but most of them are not meaningful. I understand that right now we’re focusing on other priorities, but these kinds of failures are concerning: Approved changes not being reflected: https://spa.apiviewuxtest.com/review/93790a28c3464c9f94ee3fefe8b7cff3?activeApiRevisionId=ed1f6f9f918046d9af34f3150be75fce This is not production-ready and definitely not merge-ready. I really don’t want to review this pull request again without proper tests. Relying on manual testing gives me the impression that the code wasn’t tested before reaching me, and I don’t feel comfortable approving it. To be fair, there are fixes in this pull request, such as:
My impression is that one of the bugs was introduced by hide request button if not related pull requests and overall, this is another large pull request that touches too many things and breaks others along the way. |
This issue is only introduced by my latest changes of DB, it wasn't before my latest commit. I have tested it in my local for checking the DB updates and haven't got chance to test in Ux-test for all flows before you did. Sorry about that. This is an existing issue in ApiView for quite long time, not related to any of my changes. But yes, we should fix this in recent sprints. So far, we don't have end to end testing framework established in APIVIEW codebase, manual testing can't be avoided for mocking user behaviors and all kinds of scenarios. I can list test cases I have tested and results like we did in release planner release notes to make you feel better to continue reviewing and focus on code changes only. |
You were testing in UX-test without this pr changes, it's same as testing in main. Those issues are fixed in this branch ☺ |
|
After talking with Helen Gao (@helen229) I am going to close this PR and she will follow up with individual PRs according to the themes in the issues addressed here |
close: #12211
close: #12204
close: #12215
close: #12644
Front-end: Added hasRequestedNamespaceReview flag to APIRevision model and integrated state handling in review page/options components.
UI logic: Adjusted enable/disable conditions and button state handling for namespace review requests when a review is approved or already requested.
Back-end: Added HasRequestedNamespaceReview persistence and expanded logging plus revision flag update during namespace review request flow.