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.

Sign up for GitHub

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

Jump to bottom

EU Customs Trader Portal ICS2 #239

Merged
merged 18 commits into from
Jun 22, 2024
Merged

EU Customs Trader Portal ICS2 #239

merged 18 commits into from
Jun 22, 2024

Conversation

jonrios
Copy link
Contributor

@jonrios jonrios commented May 2, 2024

No description provided.

phax
phax requested changes May 2, 2024
View reviewed changes
Copy link
Owner

@phax phax left a comment

Choose a reason for hiding this comment

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

Looks very good - thank you for that - really appreciated!

My main concerns are around the new IAS4AttachmentHandler and the mixture of UserMessage and SignalMessage. Happy to discuss that bilaterally if it helps

phase4-profile-euctp/pom.xml Outdated Show resolved Hide resolved
...profile-euctp/src/main/java/com/helger/phase4/profile/euctp/EuCtpCompatibilityValidator.java Outdated Show resolved Hide resolved
...profile-euctp/src/main/java/com/helger/phase4/profile/euctp/EuCtpCompatibilityValidator.java Outdated Show resolved Hide resolved
...profile-euctp/src/main/java/com/helger/phase4/profile/euctp/EuCtpCompatibilityValidator.java Outdated
_checkIfLegIsValid (aErrorList, aPModeLeg1, "PMode.Leg[1].");
}

// if (aPMode.getLeg2 () != null)
Copy link
Owner

Choose a reason for hiding this comment

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

If you are only allowing "one-way/push", the second leg should always be null. Is this a copy paste relict?

Copy link
Owner

Choose a reason for hiding this comment

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

Inside the PMode I saw that the 2nd leg might be present. Eventually you should also run _checkIfLegIsValid (aErrorList, aPModeLeg2, "PMode.Leg[2]."); on it?

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 added the validation for the second leg but I think we should discuss about this

phase4-profile-euctp/src/test/java/com/helger/phase4/profile/euctp/EuCtpPModeTest.java Outdated Show resolved Hide resolved
phase4-euctp-client/src/main/java/com/helger/phase4/euctp/AbstractEuctpUserMessageBuilder.java Outdated Show resolved Hide resolved
phase4-euctp-client/src/main/java/com/helger/phase4/euctp/AbstractEuctpUserMessageBuilder.java Outdated Show resolved Hide resolved
phase4-euctp-client/src/main/java/com/helger/phase4/euctp/AbstractEuctpUserMessageBuilder.java Outdated Show resolved Hide resolved
phase4-euctp-client/src/main/java/com/helger/phase4/euctp/EuctpPullRequestBuilder.java Outdated Show resolved Hide resolved
phase4-euctp-client/src/main/java/com/helger/phase4/euctp/Phase4EuCtpHttpClientSettings.java Outdated Show resolved Hide resolved
@phax
Copy link
Owner

phax commented May 14, 2024

@jonrios do my change requests make sense? Shall I implement them myself, or will you do it?

@jonrios
Copy link
Contributor Author

jonrios commented May 14, 2024

@jonrios do my change requests make sense? Shall I implement them myself, or will you do it?

Hi Philip,

thank your for your feedback and sorry for the delay.
I will implement the changes this week and we can also talk about your concerns.

Best regards
Jon

phax added a commit that referenced this pull request Jun 18, 2024
@phax
Added parseUserOrSignalMessage for #239
469c2e1
@phax
Copy link
Owner

phax commented Jun 20, 2024

Cool thanks - let me know when you are ready, so I can merge it :)

@phax phax changed the base branch from master to ics2-merge June 20, 2024 16:41
@phax phax merged commit d9d37b9 into phax:ics2-merge Jun 22, 2024
1 check passed
@phax phax added the Profile EU CTP EU Customs Trade / ICS2 label Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phax phax phax requested changes

Assignees
No one assigned
Labels
Profile EU CTP EU Customs Trade / ICS2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jonrios @phax