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

Lagt til klientMeldingId #73

Merged
merged 21 commits into from
Feb 15, 2022
Merged

Lagt til klientMeldingId #73

merged 21 commits into from
Feb 15, 2022

Conversation

jarleborsheim
Copy link
Contributor

@jarleborsheim jarleborsheim commented Jan 27, 2022

Se også PR for fiks-io-send-client-dotnet hvor klientMeldingId er tatt bort igjen som egen property.
Vi legger klientMeldingId inn som en header i stedet.

@@ -10,6 +10,7 @@ public static SendtMelding FromSentMessageApiModel(SendtMeldingApiModel sendtMel
{
return new SendtMelding(
sendtMeldingApiModel.MeldingId,
sendtMeldingApiModel.KlientMeldingID,
Copy link
Contributor

Choose a reason for hiding this comment

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

De andre suffix er pascalcase på id

Copy link
Contributor

@exoen exoen left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Security.Cryptography.X509Certificates;
Copy link
Contributor

Choose a reason for hiding this comment

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

Denne er ikke i bruk

Copy link
Contributor

@zapodot zapodot left a comment

Choose a reason for hiding this comment

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

Liten kommentar, ellers greit

model.SvarPaMelding == request.SvarPaMelding &&
model.AvsenderKontoId == request.AvsenderKontoId &&
model.MottakerKontoId == request.MottakerKontoId &&
!model.Headere.ContainsKey("klientMeldingId")),
Copy link
Contributor

Choose a reason for hiding this comment

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

Bruk konstant i stedet for String litteral

Copy link
Contributor Author

Choose a reason for hiding this comment

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

damnit
Glemte den 1 sted ja. Good catch!

@@ -28,6 +30,11 @@ public class MeldingRequest : MeldingBase

public MeldingSpesifikasjonApiModel ToApiModel()
{
if (KlientMeldingId != null && KlientMeldingId != Guid.Empty)
Copy link
Contributor

Choose a reason for hiding this comment

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

Kunne dette vært gjort i konstruktøren til MeldingsRequest? Trenger KlientMeldingId både være som prop på model og i header?

Jeg spør fordi metoden ToApiModel var i utgangspunktet uten side-effekt.

ExtractKlientMeldingId();
}

private void ExtractKlientMeldingId()
Copy link
Contributor

Choose a reason for hiding this comment

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

Ville vurdert å returnere istedenfor å bruke sideeffekt, og brukt tryParse så ikke exception blir del av logikkflyten

        private Guid ExtractKlientMeldingId()
        {
            if (Headere == null || !Headere.ContainsKey(headerKlientMeldingId))
            {
                return Guid.Empty;
            }

            if (Guid.TryParse(Headere[headerKlientMeldingId], out var id))
            {
                return id;
            }

            Logger.LogError("Kunne ikke parse KlientMeldingId funnet i header til guid. KlientMeldingId: {KlientMeldingId}", Headere[headerKlientMeldingId]);
            return Guid.Empty;
        }

@jarleborsheim jarleborsheim merged commit fb626a0 into master Feb 15, 2022
@jarleborsheim jarleborsheim deleted the jb-klientMeldingId branch February 15, 2022 09:27
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.

None yet

4 participants