-
Notifications
You must be signed in to change notification settings - Fork 191
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
New Message Service_StandaloneCatalogue Integration #220
Conversation
- Service_StandaloneCatalogue Message is added
New Message Service_StandaloneCatalogue implementation
ServiceStandaloneCatalogue Changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review the remarks I made.
/** | ||
* Passenger | ||
* | ||
* @package Amadeus\Client\RequestOptions\Fare\InformativePricing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is an exact copy of Amadeus\Client\RequestOptions\Fare\InformativePricing
, then you should just use
the existing class instead of copying it. I want as little as possible duplicate code in the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, this is not exact copy of Amadeus\Client\RequestOptions\Fare\InformativePricing.
I made some change that is require of this request. Instead if Tatoo that is array data type, we need reference that is index for each passenger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, in that case it's perhaps better to extend
from the InformativePricing class,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested by you I extended InformativePricing and created new class name ServicePassenger.
* @license https://opensource.org/licenses/Apache-2.0 Apache 2.0 | ||
*/ | ||
|
||
namespace Amadeus\Client\RequestCreator\Converter\Service; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the PSR-2 standard, there has to be an empty line between namespace declaration and use statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change done.
{ | ||
return new Struct\Service\StandaloneCatalogue($requestOptions); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the PSR-2 standard, there has to be an empty line following the closing bracket at the end of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change done
@@ -0,0 +1,61 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point of this file? It should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant the file with the tilde at the end (.php~
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
* @license https://opensource.org/licenses/Apache-2.0 Apache 2.0 | ||
*/ | ||
namespace Amadeus\Client\RequestOptions\Service\StandaloneCatalogue; | ||
use Amadeus\Client\RequestOptions\ServiceIntegratedPricingOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,124 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. No unnessary duplicate code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need FlightInfo.php here because of request parameter is different from one used in informative pricing request. We need instead of used in InformativePricing Request.
@@ -0,0 +1,60 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need index of each passenger instead of group of passenger that is used in other request. For this we need this Class file.
@@ -0,0 +1,66 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Removed this file
@@ -0,0 +1,49 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need index of each passenger instead of group of passenger that is used in other request. For this we need this Class file.
@@ -0,0 +1,47 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need index of each passenger instead of group of passenger that is used in other request. For this we need this Class file.
You will also have to make sure StyleCI passes and the Unittests pass. You will also have to add unittests, your new code should have 100% code coverage. |
I have made the changes suggested by you, please review and share your feedback. |
Dear Mika, Wish you a very happy new year! Also regarding adding unit tests we don't have much clarity on the same so kindly advice if this is mandatory or we can go without it? Else Please give us some clarity so that we can proceed ahead. As we have worked hard to achieve this, it would be joyous if you add this to your Repo. |
I'm sorry for the terrible delay, I haven't had the time recently to work on this project. I will see what I can do about the missing tests. |
Could you please provide sample XML sources of request and reply for current service? I can write tests for this PR, but I don't have access to this message on Amadeus. |
Dear Mika, No problem, hope we can manage this now. |
serviceStandaloneCatalogueReqRes.zip Thank you for your interest in current message. |
Could you please give me access to push into your branch so I will be able to add tests? |
Invitation is send. |
Oh, there's something weird with git history happened. I tried to push into master (that PR branch, but IDK what's wrong, everything resetted). Please, check this branch standalone_pricing and create new PR from that branch. I'm sorry for that. UPD: I resolved by merge directly on github. Seems everything is correct now. |
Standalone pricing
@DerMika I guess now it's finally ready to be merged! |
I will look through the last changes today or tomorrow. thanks for the hard work @therealartz and @arvind-pandey |
thank you for approval |
Hi,
Please review and share your feedback.
Thank you.