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

Implementing PNR_Ignore #198

Merged
merged 3 commits into from
Jun 1, 2018
Merged

Implementing PNR_Ignore #198

merged 3 commits into from
Jun 1, 2018

Conversation

darkmatus
Copy link
Contributor

We implementing the PNR_Ignore Call to use it in certain Webservices in combination with transactions/sessions. Actually it is not possible to ignore an PNR in a single call but this is needed and also usefull for some workflows.
What we have done:

  • implementing PNR_Ignore into existing client structure including RequestOptions, RequestCreator, ResponseHandler and Struct classes
  • adding tests for the client call and the struct class
  • add PNR_Ignore to the list of supported messages

@coveralls
Copy link

coveralls commented May 30, 2018

Coverage Status

Coverage increased (+0.0002%) to 99.858% when pulling 74abf3b on invia-de:master into 0fd1a84 on amabnl:master.

@bimusiek
Copy link
Collaborator

Currently I am using

$pnrContent = $this->client->pnrRetrieve(
    new PnrRetrieveOptions([
        'actionCode' => [
            PnrCreatePnrOptions::ACTION_IGNORE
        ]])
);

Just mentioning that it was possible before using different endpoint.

@darkmatus
Copy link
Contributor Author

My opinion is that it's better to have an own endpoint. I think it is confusing to ignore a pnr while calling pnrRetrieve

@DerMika
Copy link
Collaborator

DerMika commented May 30, 2018

I don't mind having this message implemented at all, but I agree with @bimusiek that if you need this functionality right now, the library already supports ignoring PNR's through PNR_Retrieve.

So, I don't mind merging your pull request, but you'll have to undo most of the style changes in the ClientTest class. I'll do a review in the changed files.

@bimusiek
Copy link
Collaborator

My answer was related to your statement Actually it is not possible to ignore an PNR in a single call.
Also, another endpoint means more stuff to certify.

I am not saying it is worse or better, just that it was already indeed possible to ignore PNR without this endpoint.

Copy link
Collaborator

@DerMika DerMika left a comment

Choose a reason for hiding this comment

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

Please undo all style changes fo this PR. You can make a separate pull request for any style changes, but I don't want the alignment of variables or the array declarations on a newline.

Some of the style changes you've made, like extra spaces or adding trailing comma's are fine though. But in a separate PR.

@@ -35,22 +35,24 @@ class ClientTest extends BaseTestCase
{
public function testCanCreateClient()
{
$par = new Params([
Copy link
Collaborator

Choose a reason for hiding this comment

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

Everywhere in this library we use his construct of ([ 'the' => 'array' ]), so I don't want to have this changed in just this one file.

'responseHandler' => $this->getMockBuilder('Amadeus\Client\ResponseHandler\ResponseHandlerInterface')->getMock()
]);
$par = new Params(
[
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes should be undone (I won't repeat this comment from now on)

@@ -168,14 +174,18 @@ public function testWillGetNullFromGetLastReqResHeadersWhenNoCallsWerMade()

public function testCanDoDummyPnrRetrieveCall()
{
$mockedSendResult = new Client\Session\Handler\SendResult();
$mockedSendResult = new Client\Session\Handler\SendResult();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't align the = with lines above/below, we don't do that anywhere in the library and I'd like to keep it that way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please undo similar changes in other places.

@DerMika DerMika added this to the 1.8.0 milestone May 30, 2018
@darkmatus
Copy link
Contributor Author

I've revert all style changes. Sorry for that, it's a habit.

@DerMika DerMika merged commit 1a02a0d into amabnl:master Jun 1, 2018
@DerMika
Copy link
Collaborator

DerMika commented Jun 1, 2018

Thanks for contributing!

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.

4 participants