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

Adds pagination to extension request list #1419

Merged
merged 6 commits into from
Aug 28, 2023
Merged

Conversation

Ajeyakrishna-k
Copy link
Contributor

@Ajeyakrishna-k Ajeyakrishna-k commented Aug 15, 2023

Issue: Real-Dev-Squad/website-dashboard#430

What is the change?

  • Currently we are able to filter api data based on only one value for a query param eg: ?q=status:APPROVED. This PR allows us to filter the data based on multiple values eg: ?q=status:APPROVED+PENDING
  • This PR also adds pagination support for the api. To get paginated result we would have to pass the required page size ?size=10.
  • This PR also adds a functionality to sort the data based on created time.eg: ?order=asc
  • Adds a new utility function to parse RDS query language

Note to Reviewers:

API contract:

Real-Dev-Squad/website-api-contracts#154

Test stats

Before :
Screenshot 2023-08-15 at 8 15 17 PM

After:
Screenshot 2023-08-15 at 8 16 11 PM

Screenshot 2023-08-24 at 7 53 54 PM

*Dev Tested?

  • Yes
  • No

Screenshot:

Screenshot 2023-08-28 at 12 43 35 AM

Indexes :

The following indexes needs to be created with these change:
Screenshot 2023-08-15 at 9 32 52 PM

router.get("/", authenticate, authorizeRoles([SUPERUSER, APPOWNER]), extensionRequests.fetchExtensionRequests);
router.get(
"/",
authenticate,

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.
Copy link
Contributor

@RitikJaiswal75 RitikJaiswal75 left a comment

Choose a reason for hiding this comment

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

hey Ajeyakrishna, In case we want to use multiple params we must use query params like
q=status:assigned+available we wont be using & (Moving to RDS's own query language)

@Ajeyakrishna-k
Copy link
Contributor Author

hey Ajeyakrishna, In case we want to use multiple params we must use query params like
q=status:assigned+available we wont be using & (Moving to RDS's own query language)

I took inspiration for this design from an API that's currently being used in RDS:
Screenshot 2023-08-17 at 7 08 51 AM

I don't mind making your requested changes for this PR.

But I also think we should create a separate issue to identify and migrate the other API which are not using the format that you've mentioned.

Let me know your thoughts on the same.

@Ajeyakrishna-k Ajeyakrishna-k self-assigned this Aug 18, 2023
@Ajeyakrishna-k
Copy link
Contributor Author

Ajeyakrishna-k commented Aug 20, 2023

hey Ajeyakrishna, In case we want to use multiple params we must use query params like
q=status:assigned+available we wont be using & (Moving to RDS's own query language)

@RitikJaiswal75 I need a more clarity on this.
What if we have two keys with multiple values eg: ?status=APPROVED&status=PENDING&assignee=user1&assignee=user2.
How will we handle this in the format that you have mentioned?

@prakashchoudhary07
Copy link
Contributor

Please update the API contracts for it

@Ajeyakrishna-k
Copy link
Contributor Author

Please update the API contracts for it

Ive raised a PR for the same: Real-Dev-Squad/website-api-contracts#154

@iamitprakash iamitprakash merged commit fb4b2b4 into develop Aug 28, 2023
2 of 3 checks passed
@iamitprakash iamitprakash deleted the fix/extension-requests branch August 28, 2023 20:37
@Ajeyakrishna-k Ajeyakrishna-k mentioned this pull request Aug 29, 2023
7 tasks
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

5 participants