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

Duplikatsjekk: Hensynta at forrige IM kan være ekstern #800

Merged
merged 3 commits into from
Dec 16, 2024

Conversation

bjerga
Copy link
Contributor

@bjerga bjerga commented Dec 4, 2024

Hvis vi tillater AG å sende inn IM der forrige IM er ekstern så må ikke duplikatsjekken slå inn.

Må merges inn etter #805, som løser problemene beskrevet i #800 (comment).

@bjerga bjerga requested a review from a team as a code owner December 4, 2024 15:57
Comment on lines 164 to 165
@Test
fun `skal ikke lagre duplikat inntektsmeldingskjema`() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kliss lik foregående test.

Comment on lines -165 to +156
.where { (InntektsmeldingEntitet.forespoerselId eq forespoerselId.toString()) and InntektsmeldingEntitet.skjema.isNotNull() }
.where { InntektsmeldingEntitet.forespoerselId eq forespoerselId.toString() }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dette må gjøres for å støtte en slik situasjon:

  1. AG sender inn via nav.no
  2. AG sender inn via LPS
  3. AG sender inn via nav.no, hvor IM er duplikat av 1 (skal være lov)

val nyesteIm = imRepo.hentNyesteInntektsmelding(inntektsmelding.type.id)

// TODO: Fjernes etter at vi har gått i prod med den nye innsending-flyten
val erDuplikat = nyesteIm?.erDuplikatAv(inntektsmeldingGammeltFormat) ?: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ble gjort overflødig av duplikatsjekken på skjemaklassen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Har du sett på noen av de tilfellene der denne sjekken faktisk slår inn i prod fortsatt? Ser ut som det har blitt logget "Fant duplikat av inntektsmelding." 13 ganger siste 90 dager. Jeg hadde egentlig forventet at dette skulle gått til 0 nå.

Men det har det altså ikke gjort. Forstår du hvorfor? 🙏

Kan det være på grunn av at AG har sendt inn først via LPS/Altinn og så deretter har sendt inn en kliss lik IM via nav.no? Da vil den vel kunne passere skjema-duplikat-sjekken, men ikke denne her nyeste-IM-duplikat-sjekken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jeg glemte å skrive det her, men har fått OK av Dag:
Denne PR-en åpner for at man kan sende inn én duplikat per forespørsel dersom man har sendt inn en for lenge siden, og deretter sender inn en ny etter denne endringen. Det er fordi den for lenge siden ble lagret uten skjema, så når man sender inn på ny så vil man ikke ha noe å sammenligne med. Disse tilfellene ville dog bli plukket opp av duplikatsjekken når vi lagrer inntektsmelding (som fjernes her).
Jeg gjetter at det er disse tilfellene vi ser i loggene, men jeg skal kikke i databasen for et par av dem for å bekrefte.

Copy link
Contributor

Choose a reason for hiding this comment

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

Høres bra ut! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Det var bra du catcha de loggmeldingene, for det er et tilfelle jeg ikke har tenkt på: Når AG sender inn skjema der vi bare trenger deler av informasjonen. F. eks. når Spleis ber om AGP og refusjon, så sendes fortsatt inntekt med fra frontend, men vi sender ikke inntekt videre til Spleis (forenklet forklart, det er noen finurligheter her). Når vi kun sjekker duplikatsjekk mot skjema så fører dermed ubetydelige endringer til at skjemaet ikke anses som duplikat, selv om det vi sender til Spleis vil være det.
Jeg skal gruble litt på hva som er den beste løsningen før jeg gjør noe med denne PR-en.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jeg sjekket de seks første i loggene og alle er tilfeller av det som er beskrevet i kommentaren over.

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 egen PR løser problemene som er beskrevet her. Se beskrivelse på denne PR for mer info.

@bjerga
Copy link
Contributor Author

bjerga commented Dec 5, 2024

Testet OK.
Frontend støtter ikke å sende inn ekstern IM enda, så det må testes ved en senere anledning.

Copy link
Contributor

@magnusae magnusae left a comment

Choose a reason for hiding this comment

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

val nyesteIm = imRepo.hentNyesteInntektsmelding(inntektsmelding.type.id)

// TODO: Fjernes etter at vi har gått i prod med den nye innsending-flyten
val erDuplikat = nyesteIm?.erDuplikatAv(inntektsmeldingGammeltFormat) ?: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Har du sett på noen av de tilfellene der denne sjekken faktisk slår inn i prod fortsatt? Ser ut som det har blitt logget "Fant duplikat av inntektsmelding." 13 ganger siste 90 dager. Jeg hadde egentlig forventet at dette skulle gått til 0 nå.

Men det har det altså ikke gjort. Forstår du hvorfor? 🙏

Kan det være på grunn av at AG har sendt inn først via LPS/Altinn og så deretter har sendt inn en kliss lik IM via nav.no? Da vil den vel kunne passere skjema-duplikat-sjekken, men ikke denne her nyeste-IM-duplikat-sjekken.

@bjerga bjerga merged commit 21aae81 into main Dec 16, 2024
74 checks passed
@bjerga bjerga deleted the dev/duplikatsjekk-hensynta-ekstern branch December 16, 2024 14:10
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