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

Added public endpoint for fetching subset of projects info #923

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

this-Aditya
Copy link
Member

@this-Aditya this-Aditya commented Aug 5, 2024

Added a new endpoint for fetching the project data to be used for the self-enrollment portal

Fixes #915

@Bdegraaf1234
Copy link
Member

Nice work @this-Aditya,

Some thoughts, feel free to override:

  • We already have a class MinimumprojectDetails, is that perhaps suitable to expand here?
  • We might benefit from naming these public endpoints something like api/projects/public and putting it in the ProjectResource.
  • Alternatively, I would switch to projects-lite so that any alphabetically ordered endpoint listing will group this with api/projects

I guess I just logically group this with Project a little more than is done in the current implementation. If others disagree (@yatharthranjan @mpgxvii) feel free to approve and move forward

@this-Aditya
Copy link
Member Author

Thanks for the review, @Bdegraaf1234 !
I’ll make the desired modifications. Additionally, would it be acceptable to also retrieve the updated MinimalProjectDetails when fetching from the api/projects endpoint with the minimized parameter set to true?

@Bdegraaf1234
Copy link
Member

Yeah that might be an issue, I think it would be good to check with if someone knows where we use the minimized endpoint..

Are you in the SEP meeting later today? Would be good to discuss this there

@this-Aditya
Copy link
Member Author

Okay, fine. I don't have an invite for that meeting. Could you please share the link on Slack?
Thanks.

Copy link
Member

@yatharthranjan yatharthranjan left a comment

Choose a reason for hiding this comment

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

I agree with most of Bastiaan's suggestions. Adding a new DTO instead of extending the previous one is acceptable, as this has a specific public use case. The previous one improved performance for the MP's home page (for logged-in users).

@Bdegraaf1234 I think the SEP meeting is cancelled today as a few of us are on annual leave.

@this-Aditya Please take a look at some comments below.

Copy link
Member

@yatharthranjan yatharthranjan 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. Nice work!

LGTM

@mpgxvii do you wish to review?

@yatharthranjan
Copy link
Member

Merging for now. We can create separate PRs if changes are needed

@yatharthranjan yatharthranjan merged commit 2a45f70 into dev Aug 13, 2024
6 of 7 checks passed
@this-Aditya this-Aditya deleted the project-public branch August 13, 2024 16:50
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.

Expose a new public endpoint for fetching projects
3 participants