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

Chore: koderydding del 3 #4494

Merged
merged 15 commits into from
Apr 19, 2024
Merged

Chore: koderydding del 3 #4494

merged 15 commits into from
Apr 19, 2024

Conversation

idaame
Copy link
Contributor

@idaame idaame commented Apr 18, 2024

💰 Hva skal gjøres, og hvorfor?

Skriv 1 eller 2 setninger om hvilken funksjonell endring som blir implementert. Ta med en lenke til Favro og
skriv en kort beskrivelse av hvorfor endringen skal gjøres.

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

Slette enda mer kode. Oppdaget også at kode jeg hadde forventet skulle ha vært slettet, ikke var slettet, og fortsatt var i bruk for å finne virkningstidspunkt for dødsfallbrev. Skriver derfor om her til å bruke den nye måten å generere vedtaksperioder på for å finne opphørsperiodene, som man igjen bruker for å finne virkningstidspunktet. Dette er nok ikke den smudeste måten å gjøre det på, og som jeg har kommentert føler jeg det er litt overkill. Men usikker på om det er så mye verdi å skrive om nå.

Tenker det er fornuftig å teste et dødsfallbrev i preprod før jeg merger.

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

Er det noe du er usikker på eller ønsker å diskutere? Beskriv det gjerne her eller kommenter koden det gjelder.
Bare det jeg har kommentert

✅ 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 👇

Jeg har ikke skrevet tester fordi:
Finnes allerede tester på det meste av logikken som røres, men ikke sammenkoblingen av å generere vedtaksperioder + finne virkningstidspunkt fra disse. Men, de to tingene er testet hver for seg så føler ikke det gir så mye verdi å skrive ny test:)

💬 Ønsker du en muntlig gjennomgang?

  • Ja
  • Nei

Comment on lines 254 to 260
fun hentVirkningstidspunkt(
opphørsperioder: List<Opphørsperiode>,
opphørsperioder: List<VedtaksperiodeMedBegrunnelser>,
behandlingId: Long,
) = (
opphørsperioder
.maxOfOrNull { it.periodeFom }
?.tilMånedÅr()
?: throw Feil("Fant ikke opphørdato ved generering av dødsfallbrev på behandling $behandlingId")
)
) = opphørsperioder
.maxOfOrNull { it.fom ?: TIDENES_MORGEN }
?.tilMånedÅr()
?: throw Feil("Fant ikke opphørdato ved generering av dødsfallbrev på behandling $behandlingId")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

En svakhet med å finne virkningstidspunkt på denne måten er at om man i fremtiden legger opp til fremtidig opphør/endringer så vil ikke dette nødvendigvis stemme for dødsfall-brev

Copy link
Contributor Author

@idaame idaame Apr 19, 2024

Choose a reason for hiding this comment

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

Skjønner helt ærlig ikke helt hvorfor vi må gjøre det på denne måten. Det ble tidligere gjort på en annen måte som virker mer intuitiv, men i denne PRen endret til å bruke opphørsperioder i stedet: #1901

Føler det er litt overkill, og som den andre kommentaren her sier, ikke nødvendigvis 100% riktig i fremtiden 🤔 Men syns det er skummelt å skulle endre på noe her da... så kanskje like greit å bare la det være som det er.

Copy link
Contributor

@halvorbmundal halvorbmundal Apr 19, 2024

Choose a reason for hiding this comment

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

Hmm, enig at dette er litt rart. Endringen nå er i alle fall ikke noe værre enn det var.

Kan vi eventuelt legge inn en validering på at opphørsperioden ikke er i fremtiden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tror den kan være 1 måned frem i tid, men noe mer enn det går ikke. Jeg kan legge på validering

@idaame idaame marked this pull request as ready for review April 19, 2024 07:06
@idaame idaame requested a review from a team as a code owner April 19, 2024 07:06
@@ -284,7 +285,7 @@ class BrevService(
navnAvdode = data.grunnlag.søker.navn.storForbokstavIAlleNavn(),
virkningstidspunkt =
hentVirkningstidspunkt(
Copy link
Contributor

Choose a reason for hiding this comment

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

Jeg skjønner egentlig ikke hvorfor dette ikke er data.grunnlag.søker.dødsfall?.dødsfallDato ?: throw Feil( ... )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I brevet bruker man virkningstidspunkt til å si når barnetrygden opphører fra, så tipper kanskje det isåfall ville ha vært dødsfallDato + 1 måned 🤔 men ja enig i det. Alternativt bare se på største periodeTom på andel tilkjent ytelse og plusse på 1 måned

Copy link
Contributor

@halvorbmundal halvorbmundal left a comment

Choose a reason for hiding this comment

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

Ser bra ut!

@idaame
Copy link
Contributor Author

idaame commented Apr 19, 2024

Testet og verifisert i preprod

@idaame idaame added this pull request to the merge queue Apr 19, 2024
Merged via the queue into main with commit 514b2e3 Apr 19, 2024
6 checks passed
@idaame idaame deleted the chore/koderydding-del3 branch April 19, 2024 08:44
github-merge-queue bot pushed a commit that referenced this pull request Feb 20, 2025
### 💰 Hva skal gjøres, og hvorfor?

I forbindelse med oppgave NAV-23270 kom jeg over kode som bare var brukt
i tester. Virker som disse ble glemt i denne oppgaven NAV-20565, da mye
av koden rundt ble fjernet i PR'ene #4493 og #4494
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.

2 participants