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

Obtain postal addresses in orderinfo services to show on order template #151

Closed
wants to merge 1 commit into from

Conversation

hansbak
Copy link

@hansbak hansbak commented Apr 24, 2020

new attempt, look like previous one was updated without a rebase

@danieltaylor-nz
Copy link
Member

danieltaylor-nz commented May 9, 2020

Hi Hans,
I ran this code locally and have a few questions.
I see that you have added another out parameter customerContactInfo which is an object containing all the contact mech data for that party. The pertinent values for the OrderPart are already on the out parameters, including telecomNumber, postalAddress, customerEmail etc. What would be the idea between returning a new object which contains similar but less relevant values than are already being returned?

I also see that the vendorContactInfo is pulling the postal address with type "PostalOrder". There isn't much context to the changes, but that looks wrong, and as the demo data was missing PostalPrimary values for ORG_ZIZI_RETAIL, it returned nothing during my tests. I opened a pull request to fix the demo data ( #155 ), but wouldn't a better postalContactMechPurposeId be "PostalShippingOrigin"?

Alternatively, the facilityContactInfo is already being returned, which to me seems like more relevant data for the order template.

@jonesde
Copy link
Member

jonesde commented May 9, 2020

  1. 'PostalOrder' isn't a valid contact mech purpose in the OOTB seed data, so isn't valid for vendor or customer contact info
  2. I don't know what the business case or UI design might be for the vendor contact info, but for the customer see lines 494-533 of the OrderDetail.xml file (in the actions for OrderPartSection)... a single address is useless, the point is to allow the user to select from available addresses for the OrderPart.customerPartyId and via OrderPartParty with the Customer - Ship To role

Overall I see nothing about a business level use case or UI design where these changes would be needed, or even a good idea.

@hansbak
Copy link
Author

hansbak commented May 10, 2020

Thank you for your comments, looks like this change is not wanted, closing

@hansbak hansbak closed this May 10, 2020
@jonesde
Copy link
Member

jonesde commented May 10, 2020

So rather than collaborating to improve the contribution you'd rather just drop it? Just because I don't see a business use case doesn't mean there isn't one, if you have one in mind please share... it makes it much easier to figure out what the code is trying to do in order to discuss and review it.

@hansbak
Copy link
Author

hansbak commented May 13, 2020

David, I think an order should contain postal addresses and actually email of both parties like the subject says?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants