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

fix postitions can have gangs from seperate organizations #1636

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

Conversation

magsyg
Copy link
Contributor

@magsyg magsyg commented Dec 8, 2024

Found a bug where a recruitment can have positions from gangs from other organizations, such as an UKA position in a Samf recruitment

These would not appear in the admin panel

@magsyg magsyg self-assigned this Dec 8, 2024
Copy link
Contributor

@Snorre98 Snorre98 left a comment

Choose a reason for hiding this comment

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

Det ligger også gjenger på opptak som ikke er "eid" av organisasjonen som "eier" opptaket.
Det finnes altså f.eks. en side for "Økonomi" i et Samf opptak. Se http://localhost:3000/control-panel/recruitment/1/gang/23

Bare å bytte ut gjeng id (den siste) i URLen. Alle over 17 er ikke Samf gjenger.

Så problemet du løser i denne PR-en burde i tillegg løses på et nivå over hvor du gjør det her.

backend/samfundet/models/recruitment.py Outdated Show resolved Hide resolved
@magsyg
Copy link
Contributor Author

magsyg commented Dec 10, 2024

Det ligger også gjenger på opptak som ikke er "eid" av organisasjonen som "eier" opptaket. Det finnes altså f.eks. en side for "Økonomi" i et Samf opptak. Se http://localhost:3000/control-panel/recruitment/1/gang/23

Bare å bytte ut gjeng id (den siste) i URLen. Alle over 17 er ikke Samf gjenger.

Så problemet du løser i denne PR-en burde i tillegg løses på et nivå over hvor du gjør det her.

damn nice spotta

@@ -32,19 +34,9 @@ export function RecruitmentGangAdminPage() {
// biome-ignore lint/correctness/useExhaustiveDependencies: t and navigate do not need to be in deplist
useEffect(() => {
if (recruitmentId && gangId) {
Promise.allSettled([
getRecruitmentPositionsGangForGang(recruitmentId, gangId).then((data) => {
getRecruitmentPositionsGangForGang(recruitmentId, gangId)
Copy link
Member

Choose a reason for hiding this comment

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

Gjerne bytt til å bruke react query. Docs her

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@robines resovle eller se om det kan gjøres på en annen måte. Trengs det for eksempel å bruke en useEffect i det heletatt når man bruker react query?

def get(self, request: Request, recruitment_id: int, gang_id: int) -> Response:
gang = get_object_or_404(Gang, id=gang_id)
recruitment = get_object_or_404(Recruitment, id=recruitment_id)
if recruitment.resolve_org() != gang.resolve_org():
Copy link
Contributor

Choose a reason for hiding this comment

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

Hvis dette viewet skal sørge for man ikke kan gjenger som ikke er i organisasjonene, så virker det ikke. Mulig det ikke virker fordi vi ikke har en ekte relasjon mellom gjenger og organisasjoner i seedscript?
@magsyg hvis du vet hvorfor denne checken ikke gjør det den er ment å gjøre hadde det vært nice om du lager et issue på det og kommenterer Todo og viser til det spesifikke issue nummeret.

queryFn: () => (recruitmentId && gangId ? getRecruitmentPositionsGangForGang(recruitmentId, gangId) : undefined),
enabled: !!recruitmentId && !!gangId,
});

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@robines trenger man bruke useEffect her?

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.

3 participants