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

IS-2651: Preparatory refactoring #33

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

vetlesolgaard
Copy link
Contributor

@vetlesolgaard vetlesolgaard commented Oct 2, 2024

Når vi nå skal legge til stansdato for Stans vurdering så passer det fint å definere de mulige request bodyene litt mer presist.
Tidligere måtte man visst at om vurderingen som kommer inn er et forhåndsvarsel så har alltid varselSvarfrist en verdi. På samme måte må man vite om det er en Stans vurdering som kommer inn så vil denne alltid ha en stans dato.

Mulig vi må gjøre en liten tilpasning i frontend hvor vi definerer hvilken type det er som blir sendt, før denne deployes.

  • Test at dette fungerer i dev

@vetlesolgaard vetlesolgaard requested a review from a team as a code owner October 2, 2024 06:19
@vetlesolgaard vetlesolgaard force-pushed the IS-2621-Preparatory-refactoring branch 2 times, most recently from 180ba01 to bc2c03f Compare October 2, 2024 06:49
begrunnelse: String,
document: List<DocumentComponent>,
varselSvarfrist: LocalDate?,
newVurdering: NewVurderingRequestDTO,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nå må service-laget vite om API-laget, men det er kanskje bare sånn det må bli?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Når man sender inn selve request bodyen og ikke bare verdiene har jeg sett at det er vanlig at request bodyen lever i application laget. Vi kan godt flytte den over der. Dette gir jo også mening om applikasjonslaget må forholde seg til hva som kommer inn, som den nå må gjøre

val document: List<DocumentComponent>,
val varselSvarfrist: LocalDate?,
)
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "vurderingType")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Forstår jeg det riktig som at vi her injecter feltet vurderingType som settes vha @JsonTypeName("FORHANDSVARSEL") etc for hver klasse? Hva er evt fordelen med dette vs å bare definere vurderingType for hver av de direkte som en val?

Copy link
Contributor Author

@vetlesolgaard vetlesolgaard Oct 2, 2024

Choose a reason for hiding this comment

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

Stemmer! Hvis vi definerer vurderingType direkte som val klarer ikke jackson serialisereren å finne ut hvilken type av NewVurderingRequest den skal serialisere. Slik jeg har forstått det setter vi et felt her som skal fungere som en diskriminator for hvilken av typene vi mottar.
I endepunktet har vi call.receive<NewVurderingRequestDTO>(), og da må jackson vite hvilken av subtypene til NewVurderingRequestDTO som den skal begynne å serialisere.

Normalt ligger det som type i json strukturen, men siden frontend allerede definerer det som vurderingType virket det logisk å bare etterligne frontend 😊 Så fungerer det forhåpentligvis ut av boksen

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aha skjønner, da gir det mening 👍🏼

Copy link
Collaborator

Choose a reason for hiding this comment

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

Kanskje vi kunne renamet denne til bare Vurdering en gang 🤷🏼‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enig! 🙌

@vetlesolgaard vetlesolgaard force-pushed the IS-2621-Preparatory-refactoring branch from bc2c03f to 940dc6b Compare October 2, 2024 07:18
@vetlesolgaard vetlesolgaard force-pushed the IS-2621-Preparatory-refactoring branch from 940dc6b to 9dbbf49 Compare October 2, 2024 07:19
import no.nav.syfo.domain.DocumentComponent
import java.time.LocalDate

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "vurderingType")
Copy link
Contributor

Choose a reason for hiding this comment

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

I hvilken sammenheng brukes disse annoteringene?

(vi har jo egentlig ikke pleid å ha så mye av det, så dette bør vi snakke litt om)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tror denne samtalen oppklarer det litt: #33 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hovedgrunnen til at vi her trenger annoteringer og ikke andre steder er at det vi får inn er definert som en sealed class, med et begrenset sett av subklasser. Da trenger jackson litt hjelp til å vite hvordan deserialiseringen skal foregå.

Copy link
Contributor

@andersrognstad andersrognstad 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 👍 Bare vi er komfortable med bruk av slike jackson-annoteringer, men det får kanskje være greit så lenge vi holder det til api-laget.

@vetlesolgaard
Copy link
Contributor Author

vetlesolgaard commented Oct 2, 2024

Ser bra ut 👍 Bare vi er komfortable med bruk av slike jackson-annoteringer, men det får kanskje være greit så lenge vi holder det til api-laget.

Enig i at annoteringer er kjedelig 🥲 Tror valget står mellom å enten ha annoteringer, eller å ha åpne request bodies der alle verdier som bare skal settes for spesifikke vurderinger er optional for alle typer.

@vetlesolgaard vetlesolgaard merged commit 55b9888 into main Oct 3, 2024
4 checks passed
@vetlesolgaard vetlesolgaard deleted the IS-2621-Preparatory-refactoring branch October 3, 2024 06:36
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