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

Adding all to req operation type enum #1842

Merged
merged 10 commits into from
Oct 6, 2023
Merged

Adding all to req operation type enum #1842

merged 10 commits into from
Oct 6, 2023

Conversation

muralibasani
Copy link
Contributor

Linked issue

Resolves: #900

What kind of change does this PR introduce?

  • Bug fix
  • New feature
  • Refactor
  • Docs update
  • CI update

What is the current behavior?

Describe the state of the application before this PR. Illustrations appreciated (videos, gifs, screenshots).

Currently requestOperationType does not have ALL enum. So to fetch all requests, blank is sent from FE. Rather it is better to send ALL

What is the new behavior?

Describe the state of the application after this PR. Illustrations appreciated (videos, gifs, screenshots).
Adding ALL to the enum requestOperationType

Other information:

Additional changes, explanations of the approach taken, unresolved issues, necessary follow ups, etc.

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)

@mathieu-anderson
Copy link
Contributor

Thank you! This will simplify a lot of the front end code I think. However, it also means that we need to do the front-end changes on this PR as well. The CI failures indicate why, I think ^^ The types can be patched easily, but there are some changes to be made to the underlying logic as well, and I think it's a good idea to make this PR a contained package.

I can push additional commits once the backend changes have been reviewed and approved.

aindriu-aiven
aindriu-aiven previously approved these changes Oct 4, 2023
Copy link
Contributor

@aindriu-aiven aindriu-aiven left a comment

Choose a reason for hiding this comment

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

LGTM

@mathieu-anderson
Copy link
Contributor

mathieu-anderson commented Oct 5, 2023

@muralibasani this commit should address all the FE issues a4c2955

However, I noticed some significant API misconduct: adding operationType=ALL to API calls seems to return nothing:

Screen.Recording.2023-10-05.at.16.11.50.mov
Screen.Recording.2023-10-05.at.16.26.15.mov

@mathieu-anderson
Copy link
Contributor

I also requested a review from @programmiri because I might have overlooked a spot where this change to operationType should also be reflected ^^

@mathieu-anderson
Copy link
Contributor

@muralibasani I think the commit ea3e3d6 fixed the issue I mentioned, thank you! I would still wait on a review by @programmiri to merge though ^^

aindriu-aiven
aindriu-aiven previously approved these changes Oct 6, 2023
@@ -227,7 +227,7 @@ public Iterable<AclRequests> findAclRequestsByExample(
if (requestor != null && !requestor.isEmpty()) {
request.setRequestor(requestor);
}
if (requestOperationType != null) {
if (requestOperationType != null && !requestOperationType.value.equalsIgnoreCase("all")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick but "all" should probably be repalced by requestOperationType != RequestOperationType.ALL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aindriu-aiven updated.

Copy link
Contributor

@aindriu-aiven aindriu-aiven left a comment

Choose a reason for hiding this comment

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

LGTM

@muralibasani muralibasani merged commit 3d5242a into main Oct 6, 2023
21 checks passed
@muralibasani muralibasani deleted the issue900 branch October 6, 2023 11:35
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.

refactor: Align options and handling for requestOperationType and requestStatus
4 participants