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

Redirect to active samf recruitment #1861

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

Snorre98
Copy link
Contributor

@Snorre98 Snorre98 commented Apr 2, 2025

What

  • Add view for fetching samfundet active recruitments. See http://localhost:8000/api/active-recruitment/samfundet/

  • Adds special loader, checking if there are multiple samf recruitment, if there is one navigate to the one. When there is many it goes to the public page with a list of recruitments.

Why

We don't know if we actually want to promote other recruitments on the website. This is something we need to discuss with opptaksforum, ISFiT and UKA.

the solution I implemented here still enables the user to navigate to the public page with list of recruitment cards, but only if there are multiple Samf recruitments

closes #1475

Snorre98 added 5 commits April 2, 2025 09:36
…tment specific page if there is only one recruitment
… if there is one navigate to the one. When there is many it goes to the public page with a list of recruitments.

A bit sceptical of this solution.
@Snorre98 Snorre98 self-assigned this Apr 2, 2025
Copy link
Contributor

@Mathias-a Mathias-a left a comment

Choose a reason for hiding this comment

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

Comment on lines 64 to 67
@action(detail=False, methods=['get'], url_path='samfundet')
def get_active_samf_recruitments(self, request: Request, **kwargs: Any) -> Response:
try:
samfundet_org = Organization.objects.get(name='Samfundet')
Copy link
Contributor

Choose a reason for hiding this comment

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

Finn eller flytt name inn i en konstant noe sted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sånn her lissom? 8ff2c70

Copy link
Contributor Author

@Snorre98 Snorre98 Apr 8, 2025

Choose a reason for hiding this comment

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

^det ble feil,. Dette funker: e81d635

Comment on lines 69 to 73
# Get active recruitments for Samfundet, using the overriden get_queryset method
active_samfundet_recruitments = self.get_queryset().filter(organization=samfundet_org)

if not active_samfundet_recruitments:
return Response({'message': 'No active recruitment for Samfundet'}, status=status.HTTP_404_NOT_FOUND)
Copy link
Contributor

Choose a reason for hiding this comment

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

Syntes det virker riktigere å returnere en tom liste her enn en 404 feil?

Copy link
Contributor

Choose a reason for hiding this comment

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

Dvs bare fjerne dette?

Copy link
Contributor Author

@Snorre98 Snorre98 Apr 8, 2025

Choose a reason for hiding this comment

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

Tror jeg er uenig. Er det ikke bedre at det er tydelig at det ikke finnes opptak, i stede for å returnere en tom liste. Jeg føler det å returnere en tom liste mer sannsynlig gjøre at fremtidige devs kan komme til å tro at det er noe feil i koden hvis det returneres en tom liste sammenlignet med melding som sier at det ikke finnes opptak som etterspørres her.

Nå gjorde jeg forresten en delvis urelatert endring (her fe23411)hvor jeg sjekker om det eksistere ne organisasjon med navn Samfundet med en if-statement istedefor en try/catch. Føler det er litt mer lesbart og gjøre kan "flyte" litt bedre enn exceptions.

Jeg mener i hvertfall man burde sjekke om organisasjonen med navn Samfundet eksistere, slik at får en feil hvis Samf av en eller annen grunn heter noe annet, f.eks. Samf eller Studentersamfundet i Trondhjem.

Comment on lines 137 to 146
route={
activeSamfRecruitments.length === 1
? // goes to the one samf recruitment if there is only one
reverse({
pattern: ROUTES.frontend.organization_recruitment,
urlParams: { recruitmentId: activeSamfRecruitments[0].id },
})
: // goes to the page with recruitment cards if there is multiple samf recruitments
ROUTES.frontend.recruitment
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Dette kan gå i en konstant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done d70b36d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

og med en oppfølger her 1ec48cf

Comment on lines +39 to +40
try {
const activeSamfRecruitments = await getActiveSamfRecruitments();
Copy link
Contributor

Choose a reason for hiding this comment

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

tanstack?

Copy link
Contributor Author

@Snorre98 Snorre98 Apr 3, 2025

Choose a reason for hiding this comment

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

Det er ikke tsx, altså, kan ikke kjøre en hook her

Comment on lines 139 to 133
/>
{showActiveRecruitments && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nå forsvinner knappen når det ikke er opptak. Det er ikke ønsket, man burde kunne trykke seg inn når som helst for å få info om opptak generelt, og når neste opptak evt. er

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Endret her 1c52c77

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.

Change recruitment home page
2 participants