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

Nav 24039 forskyv vilkår lov 2025 #1050

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

thoalm
Copy link
Contributor

@thoalm thoalm commented Jan 27, 2025

💰 Hva skal gjøres, og hvorfor?

https://favro.com/organization/98c34fb974ce445eac854de0/1844bbac3b6605eacc8f5543?card=NAV-24039.

Implementerer forskyvningslogikk for ny lovendring. Den nye logikken skal samsvare med 2021 sin forskyvning. Vi har valgt å skille dette ut i en egen pakke for ryddighets skyld, men det er mye duplikat kode.

🔎️ Er det noe spesielt du ønsker tilbakemelding om?

Jeg (Gard) er på ganske tynn is domenemessig, ta helst en nøyere titt om det er noe jeg har misforstått. Jeg er spesielt usikker på om forskyvningen skal forholde seg til den totale vilkårsperioden eller ikke.

Gi gjerne også en ekstra tanke på plassering av kode og navngivning.

✅ Checklist

Har du husket alle punktene i listen?

  • Jeg har testet mine endringer i henhold til akseptansekriteriene 🕵️
  • Jeg har config- eller sql-endringer. I så fall, husk manuell deploy til miljø for å verifisere endringene.
  • Jeg har skrevet tester. Hvis du ikke har skrevet tester, beskriv hvorfor under 👇

💬 Ønsker du en muntlig gjennomgang?

  • Ja
  • Nei

@GardOS GardOS marked this pull request as ready for review January 28, 2025 13:23
@GardOS
Copy link
Contributor

GardOS commented Jan 28, 2025

Quality Gate Failed Quality Gate failed

Failed conditions 5.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Vet ikke hva vi skal gjøre med denne. Hvis vi ignorerer den nå, vil den mase fremover?

@GardOS GardOS force-pushed the NAV_24039_forskyv_vilkår_lov_2025 branch from b01ac6a to 50f8c1e Compare January 29, 2025 12:50
import no.nav.familie.tidslinje.tilTidslinje
import no.nav.familie.tidslinje.utvidelser.tilPerioderIkkeNull

object ForskyvVilkårEtterFebruar2025 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tror kanskje det er lurt å gjøre noe med navngivningen av fila og objectet her. Vi har brukt ordet "før" på det som eksisterer fra før, men tenker at vi for den nye og fremtidige ikke burde bruke ordet "etter", men noe mer spesifikt som peker på lovendringen. F.eks: ForskyvVilkårLovendringFebruar2025 eller LovendringFebruar2025VilkårForskyver. Vet ikke om jeg ble så fornøyd med noen av de navnene heller, men tenker ihvertfall at vi bør styre unna "Etter" i navnet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Jeg er enig i vi burde gå vekk fra "etter". Sliter med å komme på navn selv gitt. Ser ut som at det blir ForskyvVilkårLovendringFebruar2025 med mindre jeg kommer på noe lurt.

@@ -67,7 +68,7 @@ fun PersonResultat.forskyvVilkårResultater(
): Map<Vilkår, List<Periode<VilkårResultat>>> =
when (lovverk) {
Lovverk.FØR_LOVENDRING_2025 -> ForskyvVilkårFørFebruar2025.forskyvVilkårResultater(personResultat = this)
Lovverk.LOVENDRING_FEBRUAR_2025 -> TODO()
Lovverk.LOVENDRING_FEBRUAR_2025 -> ForskyvVilkårEtterFebruar2025.forskyvVilkårResultater(this.vilkårResultater.toList())
Copy link
Contributor

Choose a reason for hiding this comment

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

Tenker det er ryddig på sende inn enten List<VilkårResultat> eller PersonResultat for både Lovverk.FØR_LOVENDRING_2025 og Lovverk.LOVENDRING_FEBRUAR_2025. Litt nitpick, men synes det blir litt ryddigere da.

Copy link
Contributor

Choose a reason for hiding this comment

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

Redd jeg ikke helt forstår. Parameter-typen er vel allerede PersonResultatog List<VilkårResultat>? Eller tenker du på hvordan man sender det?

Copy link
Contributor

@bragejahren bragejahren left a comment

Choose a reason for hiding this comment

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

Synes dette ser bra ut jeg! Det blir mye duplikat kode og det er noe med strukturen som føles litt "off", men det er jo satt opp likt for eksisterende forskyvning så 🤷 Vanskelig å sette fingeren på det 🤔

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
5.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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.

4 participants