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

might refactor views #1736

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

might refactor views #1736

wants to merge 10 commits into from

Conversation

Snorre98
Copy link
Contributor

@Snorre98 Snorre98 commented Mar 3, 2025

Taking a stab at improving how we do views. A refactor like this would obviously require a lot of changes in frontend as well. Dont mind the permission mixins. Dont know if they work
❓ Is there a reason why we are not doing views with actions?

The idea here is to substitute views related to applications with one RecruitmentAppicationViewSet (the same could be done for RecruitmentPosition related views), using actions. In future issues/PRs we could start deleting the substituted views, and implement the newly created endpoints in frontend. This to avoid huge merge-conflicts.

Why one ModelViewSet?

I believe this way, with one ModelViewSet for every model and actions, we leverage Django REST Framework to create actual good API routes. In addition it makes code more readable as all view-actions endpoints under one "parent-endpoint" are collected in the same class, which in this case is RecruitmentAppicationViewSet.

It allows for structured implementation like this:

/api/recruitment-application/?recruitment=2

/api/recruitment-application/for-gang/?gang=2&recruitment=1

/api/recruitment-application/for-section/?section=2&recruitment=1

/api/recruitment-application/recruiter-withdraw/?application=198b1903-b764-477a-a08c-7d966148688e

In some cases it will probably be best to use single simple views, but I believe the implementation in this PR is reasonable.

Drawbacks

  • This will mess with existing tests.
  • Large ModelViewSets' might not be needed for every case.
  • This will require a large refactor in frontend, to use the existing views.

@Snorre98 Snorre98 added the question Further information is requested. label Mar 3, 2025
@Snorre98 Snorre98 self-assigned this Mar 3, 2025
@Snorre98 Snorre98 linked an issue Mar 3, 2025 that may be closed by this pull request
@Snorre98
Copy link
Contributor Author

Snorre98 commented Mar 5, 2025

image

@Snorre98
Copy link
Contributor Author

Snorre98 commented Mar 5, 2025

image

@Mathias-a jeg syns det virker som fordelen med ModelViewSet som det er implementert i denne PR-en har fordelen at man får et mer strukturert API, både fordi det benytter DRF route generering, men også fordi jeg ser for meg utviklere mer sannsynlig kommer til å lage endepunkter som gir mening gitt at det ser relatert endepunkter definert i et eneste ModelViewSet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

might refactor views 🤡
1 participant