Refactor/rest like endpoints #180

Sirze01 · 2023-07-22T17:41:23Z

Closes #143

First of all kudos to @BrunoRosendo for the great groundwork.
This PR refactors some of the endpoints being used and adds support for URL queries in the documentation, as, as of now, some endpoints rely on URL queries to filter the returned contents:

Next is a list of the affected endpoints:

accounts
- POST /changePassord/{id} -> POST /{id}/password
- POST /new -> POST
auth
- POST /new -> POST
events
- GET /category -> GET ?category=Great Events (Missing documentation for query parameters)
- POST /new -> POST
- PUT /{idEvent}/addTeamMember/{idAccount} -> PUT /{idEvent}/team/{idAccount}
- PUT /{idEvent}/removeTeamMember/{idAccount} -> DELETE /{idEvent}/team/{idAccount}
generations
- GET -> GET /years
- POST /new -> POST
posts
- POST new -> POST
projects
- POST /new -> POST
- PUT /{idProject}/addTeamMember/{idAccount} -> PUT /{idProject}/team/{idAccount}
- PUT /{idProject}/removeTeamMember/{idAccount} -> DELETE /{idProject}/team/{idAccount}
- PUT /{idEvent}/addHallOffFameMember/{idAccount} -> PUT /{idEvent}/hallOfFameMember/{idAccount}
- PUT /{idEvent}/removeHallOfFameMember/{idAccount} -> DELETE /{idEvent}/hallOfFameMember/{idAccount}

Review checklist

Properly documents API changes in the corresponding controller test
Contains enough appropriate tests
Behavior is as expected
Clean, well structured code

github-actions · 2023-07-22T17:47:12Z

Check the documentation preview: https://64bc161bc1e594100f3b7140--niaefeup-backend-docs.netlify.app

Sirze01 · 2023-07-22T18:01:14Z

Some endpoints are somewhat rule-breakers.
I thought of some alternatives, if you guys think we should change them, let me know.

I think having different operations to handle the team and project memberships is somewhat of an anti-pattern in REST terms. Having the operations seems to be way too imperative, almost resembling Remote Procedure Calls (RPC).

Events
- Single PUT /{idEvent}/team (with a list "team" resource for simplification), the list of members would be rewritten with every update
- Patching the Event object with the new team members list
- Just rewriting the whole event object
Projects
- Patch to the project object with the new value of isArchived
- PUT /{id}/ with the modified body in the case of the archival/unarchival

This is me being pedantic at this point, as I see value in these exceptions, making the endpoints easier to interact with.
Let me know your opinion.

github-actions · 2023-07-22T18:07:53Z

Check the documentation preview: https://64bc1af5ee3868146d26cade--niaefeup-backend-docs.netlify.app

BrunoRosendo · 2023-07-26T01:12:27Z

I haven't reviewed but I want to warn that after #139 this PR needs to address the API on the new endpoints. Let's discuss the team decision in the next meeting

LuisDuarte1 · 2023-07-26T16:42:25Z

Some endpoints are somewhat rule-breakers. I thought of some alternatives, if you guys think we should change them, let me know.

I think having different operations to handle the team and project memberships is somewhat of an anti-pattern in REST terms. Having the operations seems to be way too imperative, almost resembling Remote Procedure Calls (RPC).

Events

Single PUT /{idEvent}/team (with a list "team" resource for simplification), the list of members would be rewritten with every update

Patching the Event object with the new team members list

Just rewriting the whole event object

Projects

Patch to the project object with the new value of isArchived

PUT /{id}/ with the modified body in the case of the archival/unarchival

This is me being pedantic at this point, as I see value in these exceptions, making the endpoints easier to interact with. Let me know your opinion.

I think that it's not pedantic at all and you approach is simpler imo, it simplifies the backend by reducing the number of endpoints, by having the side-effect of sometimes making more api calls (eg.: to add a team member, you have to get the current list and then make the request to rewrite the list of members with the new member. In most situations there's no additional overhead, but in some cases it'll add a request of overhead). I think we need more opinion on this cc: @MRita443

BrunoRosendo

Good job with the refactor!

Thoughts/Answers to your comment:

As for adding members to the teams, I think we could change to POST /events/{id}/team and pass an id in the body, resembling the creation of a new team member. I think this is more RESTful and keeps the API simpler to use
As for project archival/unarchival, if we want to be very strict about REST norms then we could erase it since we can change it using the regular PUT. The reason I proposed these endpoints at the time was that I could see a clear purpose for them (e.g. clicking an archive button instead of editing a form that updates the project). We can also say that updating and archiving entities are different things (independently of implementation), there seems to be a discussion on this https://stackoverflow.com/questions/16201905/restful-archiving-of-entities-in-webapi
Personaly, I prefer having an exception to REST and keeping the API easily understandable, just like GitHub and Reddit do in some cases.

BrunoRosendo · 2023-07-27T13:07:23Z

src/test/kotlin/pt/up/fe/ni/website/backend/controller/EventControllerTest.kt

+    @DisplayName("GET events?category={category}")
+    inner class GetEventsByCategory {


I think all these tests should be moved into GetEvents

This got me thinking that we might want to have more filters on the BE (e.g. archived posts) but I think we can wait to see how FE wants to implement filters

I think all these tests should be moved into GetEvents

Will do!

Sirze01 · 2023-07-28T12:47:27Z

Good job with the refactor!

Thoughts/Answers to your comment:

* As for adding members to the teams, I think we could change to POST /events/{id}/team and pass an id in the body, resembling the creation of a new team member. I think this is more RESTful and keeps the API simpler to use

* As for project archival/unarchival, if we want to be very strict about REST norms then we could erase it since we can change it using the regular PUT. The reason I proposed these endpoints at the time was that I could see a clear purpose for them (e.g. clicking an archive button instead of editing a form that updates the project). We can also say that updating and archiving entities are different things (independently of implementation), there seems to be a discussion on this https://stackoverflow.com/questions/16201905/restful-archiving-of-entities-in-webapi
  Personaly, I prefer having an exception to REST and keeping the API easily understandable, just like [GitHub](https://docs.github.com/en/free-pro-team@latest/rest/issues/issues?apiVersion=2022-11-28#lock-an-issue) and [Reddit](https://www.reddit.com/dev/api/#POST_api_mod_conversations_:conversation_id_archive) do in some cases.

Regarding team updates
I would go even further and instead of taking only a member ID in the body, take an array of IDs to support batch updates, (Adding/removing more than one in a "commit" action in the frontend)
Regarding the archive/unarchive
I think we should leave the endpoints as they are now.

BrunoRosendo · 2023-08-07T18:07:59Z

#64 was merged, could you please refactor the endpoints too? It's the exact same thing as adding team members

LuisDuarte1 · 2023-08-30T15:41:39Z

@Sirze01 can you look into this please?

Sirze01 · 2023-10-07T15:04:35Z

Closes #143

First of all kudos to @BrunoRosendo for the great groundwork. This PR refactors some of the endpoints being used and adds support for URL queries in the documentation, as, as of now, some endpoints rely on URL queries to filter the returned contents:

Next is a list of the affected endpoints:

* accounts
  
  * POST /changePassord/{id} -> POST /{id}/password
  * POST /new -> POST

* auth
  
  * POST /new -> POST

* events
  
  * GET /category -> GET ?category=Great Events (Missing documentation for query parameters)
  * POST /new -> POST
  * PUT /{idEvent}/addTeamMember/{idAccount} -> PUT /{idEvent}/team/{idAccount}
  * PUT /{idEvent}/removeTeamMember/{idAccount} -> DELETE /{idEvent}/team/{idAccount}

* generations
  
  * GET -> GET /years
  * POST /new -> POST

* posts
  
  * POST new -> POST

* projects
  
  * POST /new -> POST
  * PUT /{idProject}/addTeamMember/{idAccount} -> PUT /{idProject}/team/{idAccount}
  * PUT /{idProject}/removeTeamMember/{idAccount} -> DELETE /{idProject}/team/{idAccount}
  * PUT /{idEvent}/addHallOffFameMember/{idAccount} -> PUT /{idEvent}/hallOfFameMember/{idAccount}
  * PUT /{idEvent}/removeHallOfFameMember/{idAccount} -> DELETE /{idEvent}/hallOfFameMember/{idAccount}

Review checklist

* [x]  Properly documents API changes in the corresponding controller test

* [x]  Contains enough appropriate tests

* [x]  Behavior is as expected

* [x]  Clean, well structured code

Updated the PR description.
The PR in the frontend seems to be updated still.

github-actions · 2023-10-07T15:14:14Z

Check the documentation preview: https://652175c3fcd9760d2429564a--niaefeup-backend-docs.netlify.app

BrunoRosendo

really small detail!

src/main/kotlin/pt/up/fe/ni/website/backend/controller/ProjectController.kt

BrunoRosendo

Great job!

github-actions · 2023-10-09T23:35:02Z

Check the documentation preview: https://65248e239ed0e50b7944894c--niaefeup-backend-docs.netlify.app

LuisDuarte1

LGTM, great work 🚀

github-actions · 2023-10-10T09:10:00Z

Check the documentation preview:

Sirze01 self-assigned this Jul 22, 2023

Sirze01 linked an issue Jul 22, 2023 that may be closed by this pull request

Refactor endpoints to follow a more "REST-like" approach #143

Closed

Sirze01 marked this pull request as ready for review July 22, 2023 18:01

Sirze01 closed this Jul 22, 2023

Sirze01 reopened this Jul 22, 2023

MRita443 requested review from MRita443 and LuisDuarte1 July 26, 2023 15:36

BrunoRosendo requested changes Jul 27, 2023

View reviewed changes

Sirze01 added 5 commits October 6, 2023 23:57

Refactor accounts endpoints

e8f8a5e

Refactor auth endpoints

5a3b31a

Refactor events endpoints

af590b3

Add documentation support for query parameters

d98d23a

Refactor generations endpoints

b7f9acc

Sirze01 force-pushed the refactor/rest-like-endpoints branch 2 times, most recently from b445b21 to 80b7e39 Compare October 6, 2023 23:41

Refactor posts endpoints

f281ac5

Sirze01 force-pushed the refactor/rest-like-endpoints branch from 80b7e39 to f281ac5 Compare October 6, 2023 23:48

Sirze01 added 2 commits October 7, 2023 15:48

Merge get all events and get events by category tests

bcd30ae

Refactor hall of fame endpoints

06838cb

Sirze01 requested a review from BrunoRosendo October 7, 2023 15:06

BrunoRosendo requested changes Oct 9, 2023

View reviewed changes

src/main/kotlin/pt/up/fe/ni/website/backend/controller/ProjectController.kt Outdated Show resolved Hide resolved

Normalize hall of fame endpoints

2f4c835

Sirze01 requested a review from BrunoRosendo October 9, 2023 23:25

BrunoRosendo approved these changes Oct 9, 2023

View reviewed changes

LuisDuarte1 approved these changes Oct 10, 2023

View reviewed changes

LuisDuarte1 merged commit cf4c310 into develop Oct 10, 2023
3 checks passed

LuisDuarte1 deleted the refactor/rest-like-endpoints branch October 10, 2023 09:02

BrunoRosendo mentioned this pull request Mar 30, 2024

43 events gallery of photos #203

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/rest like endpoints #180

Refactor/rest like endpoints #180

Sirze01 commented Jul 22, 2023 •

edited

Loading

github-actions bot commented Jul 22, 2023

Sirze01 commented Jul 22, 2023

github-actions bot commented Jul 22, 2023

BrunoRosendo commented Jul 26, 2023

LuisDuarte1 commented Jul 26, 2023 •

edited

Loading

BrunoRosendo left a comment

BrunoRosendo Jul 27, 2023

BrunoRosendo Jul 27, 2023

Sirze01 Jul 28, 2023

Sirze01 commented Jul 28, 2023

BrunoRosendo commented Aug 7, 2023

LuisDuarte1 commented Aug 30, 2023

Sirze01 commented Oct 7, 2023 •

edited

Loading

Review checklist

github-actions bot commented Oct 7, 2023

BrunoRosendo left a comment

BrunoRosendo left a comment

github-actions bot commented Oct 9, 2023

LuisDuarte1 left a comment

github-actions bot commented Oct 10, 2023

		@DisplayName("GET events?category={category}")
		inner class GetEventsByCategory {

Refactor/rest like endpoints #180

Refactor/rest like endpoints #180

Conversation

Sirze01 commented Jul 22, 2023 • edited Loading

Review checklist

github-actions bot commented Jul 22, 2023

Sirze01 commented Jul 22, 2023

github-actions bot commented Jul 22, 2023

BrunoRosendo commented Jul 26, 2023

LuisDuarte1 commented Jul 26, 2023 • edited Loading

BrunoRosendo left a comment

Choose a reason for hiding this comment

BrunoRosendo Jul 27, 2023

Choose a reason for hiding this comment

BrunoRosendo Jul 27, 2023

Choose a reason for hiding this comment

Sirze01 Jul 28, 2023

Choose a reason for hiding this comment

Sirze01 commented Jul 28, 2023

BrunoRosendo commented Aug 7, 2023

LuisDuarte1 commented Aug 30, 2023

Sirze01 commented Oct 7, 2023 • edited Loading

Review checklist

github-actions bot commented Oct 7, 2023

BrunoRosendo left a comment

Choose a reason for hiding this comment

BrunoRosendo left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 9, 2023

LuisDuarte1 left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 10, 2023

Sirze01 commented Jul 22, 2023 •

edited

Loading

LuisDuarte1 commented Jul 26, 2023 •

edited

Loading

Sirze01 commented Oct 7, 2023 •

edited

Loading