Skip to content
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

[AC-2303] Implement approveAllRequests method #9031

Merged
merged 69 commits into from May 24, 2024

Conversation

r-tome
Copy link
Contributor

@r-tome r-tome commented May 3, 2024

Type of change

- [ ] Bug fix
- [X] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Implement method in Api service to post a request to the server endpoint implemented in bitwarden/server#4077

Code changes

  • file.ext: Description of what was changed and why

Screenshots

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

r-tome and others added 30 commits April 17, 2024 13:03
The import path of `PendingAuthRequestView` in
`OrganizationAuthRequestApiService` was pointing to the wrong place. I
think this file was just recently moved, and the import didn't get
updated.
@r-tome r-tome changed the base branch from ac/ac-2302/move-device-approve-deny-into-a-service to ac/ac-2328/bulk-getresetpassworddetails May 14, 2024 15:25
@r-tome r-tome requested a review from addisonbeck May 17, 2024 12:55
@r-tome r-tome marked this pull request as ready for review May 17, 2024 12:55
@r-tome
Copy link
Contributor Author

r-tome commented May 17, 2024

@addisonbeck Your PR isn't ready yet but this way I can get feedback from you early

Comment on lines 13 to 21
export class AdminAuthRequestUpdateWithIdRequest extends AdminAuthRequestUpdateRequest {
constructor(
public id: string,
public requestApproved: boolean,
public encryptedUserKey?: string,
) {
super(requestApproved, encryptedUserKey);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These property names don't line up with the what the server expects here

I named them the way I did for simplicity, the properties are named that way all the way through to the table object. I'd like to keep the server names if possible and rename these fields in clients. I think you'd just need to avoid extending the existed request model and declaring a new one instead.

The model names are also a bit different, the server one is called OrganizationAuthRequestUpdateManyRequestModel.

Comment on lines 41 to 53
async approvePendingRequests(
organizationId: string,
items: AdminAuthRequestUpdateWithIdRequest[],
): Promise<void> {
await this.apiService.send(
"POST",
`/organizations/${organizationId}/auth-requests/approve`,
new BulkApproveAuthRequestsRequest(items),
true,
false,
);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The API method is a bit more general purpose than this. It also supports denying auth requests, and is at a slightly different URL. It's the default POST on the organization auth request controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've deleted the BulkApproveAuthRequestsRequest class and adjusted the endpoint name

@r-tome r-tome requested a review from addisonbeck May 22, 2024 12:05
addisonbeck
addisonbeck previously approved these changes May 23, 2024
Base automatically changed from ac/ac-2328/bulk-getresetpassworddetails to main May 24, 2024 10:20
@r-tome r-tome dismissed addisonbeck’s stale review May 24, 2024 10:20

The base branch was changed.

@r-tome r-tome requested a review from a team as a code owner May 24, 2024 10:20
@r-tome r-tome requested a review from addisonbeck May 24, 2024 10:20
@r-tome r-tome merged commit a49e7bb into main May 24, 2024
19 checks passed
@r-tome r-tome deleted the ac/ac-2303/implement-approveallrequests-method branch May 24, 2024 14:48
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.

None yet

3 participants