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

Added support for Fee Options in Fare_MasterPricerTravelBoardSearch #157

Merged
merged 5 commits into from
Mar 17, 2018

Conversation

zervel
Copy link
Contributor

@zervel zervel commented Mar 15, 2018

Added 2 option in MasterPricer:
5.69 Operation: 06.3 Fee Option - Sorting with/without Fees
5.71 Operation: 06.5 Fee Option - Add/Exempt airline ticketing fees by sub-
code

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.85% when pulling 2c35a01 on zervel:master into e3318fb on amabnl:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.85% when pulling 2c35a01 on zervel:master into e3318fb on amabnl:master.

@coveralls
Copy link

coveralls commented Mar 15, 2018

Coverage Status

Coverage increased (+0.01%) to 99.85% when pulling eaff3b0 on zervel:master into e3318fb on amabnl:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.85% when pulling 2c35a01 on zervel:master into e3318fb on amabnl:master.

@DerMika
Copy link
Collaborator

DerMika commented Mar 15, 2018

Hi,

Thanks for contributing!

The only problem I see is that the request options object MPFeeOption seems to be overly complex.

It has always been a goal of this library to make building the request as easy as possible, and for the MPFeeOption as you've built it now, I wonder what can be done to make it simpler to use.

It doesn't have to match the XML message request structure, in fact if you can make it easier to use (while still providing all possibilities provided by Amadeus' API), I would prefer that.

For example:

  • constant values which only have one option should not have to be provided by the user of this library, I prefer if such values are automatically filled (e.g. MonetaryDetails::TYPE_QUALIFIER_ASSOCIATED_FEE)
  • a level of XML nodes which is redundant for providing options should not be necessary to put in the request options, as it requires extra work by users of the library to implement.

So, can you please try to simplify the request options as much as possible (while keeping all options open that are possible according to the documentation)?

@zervel
Copy link
Contributor Author

zervel commented Mar 16, 2018

Hi,
thank you for your feedback 👍 .
i simplified the fee option request.

@DerMika
Copy link
Collaborator

DerMika commented Mar 16, 2018

Looks much better! I will check this weekend if the request built matches the Amadeus docs, and then I'll merge.

One small error I noticed: in Amadeus\Client\Struct\Fare\MasterPricer\MonetaryDetails you're not passing the location property of MonetaryDetailsRequest.

@DerMika DerMika changed the title Implemented attaching feeOption at MasterPricerTravelBoardSearch Added support for Fee Options in Fare_MasterPricerTravelBoardSearch Mar 17, 2018
@DerMika DerMika merged commit 227ab83 into amabnl:master Mar 17, 2018
@DerMika
Copy link
Collaborator

DerMika commented Mar 17, 2018

Thanks again for your contribution!

@DerMika
Copy link
Collaborator

DerMika commented Mar 17, 2018

I see you've added an entry to the changelog but didn't mention your name. Do you prefer if I don't add your name? (because otherwise I will!)

@DerMika DerMika added this to the 1.7.0 milestone Mar 18, 2018
@zervel
Copy link
Contributor Author

zervel commented Mar 18, 2018

Sure, you can add my name i forgot to add this :)

@DerMika
Copy link
Collaborator

DerMika commented Apr 30, 2018

Version 1.7.0 has just been released with this PR included!

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

Successfully merging this pull request may close these issues.

3 participants