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

Support assigning API products to Apps on creation #248

Merged
merged 19 commits into from
Mar 13, 2023

Conversation

phdhiren
Copy link
Contributor

@phdhiren phdhiren commented Mar 3, 2023

Fixes #214

@phdhiren phdhiren marked this pull request as ready for review March 8, 2023 15:36
@mxr576
Copy link
Contributor

mxr576 commented Mar 9, 2023

Sorry, but I still have concerns with this design change that are not answered yet
https://github.com/apigee/apigee-client-php/pull/248/files#r1129955554

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #248 (2e89038) into 2.x (a6a24db) will increase coverage by 0.14%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x     #248      +/-   ##
============================================
+ Coverage     89.01%   89.16%   +0.14%     
- Complexity     1646     1649       +3     
============================================
  Files           334      330       -4     
  Lines          4379     4374       -5     
============================================
+ Hits           3898     3900       +2     
+ Misses          481      474       -7     
Flag Coverage Δ
unittests 89.16% <33.33%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Api/Management/Entity/App.php 89.18% <33.33%> (-10.82%) ⬇️

... and 4 files with indirect coverage changes

@giteshk giteshk requested a review from mxr576 March 10, 2023 15:41
@phdhiren
Copy link
Contributor Author

@giteshk @mxr576

I've added the exception so the function gets called only to create an app and throw for the update app.

if (!$this->appId) {
$this->initialApiProducts = $initialApiProducts;
} else {
throw new \Exception('This method is only supported for creating a new app.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a \LogicException() since calling it is a "mistake made by the developer".

(Although we know this method should only exist on New app entities for which we do not have an object type...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been taken care. Thank you for pointing it out.

@mxr576
Copy link
Contributor

mxr576 commented Mar 10, 2023

Thanks. What about adding test coverage?

@phdhiren phdhiren merged commit 7bae0d5 into apigee:2.x Mar 13, 2023
@phdhiren phdhiren added this to the 2.0.19 milestone Mar 14, 2023
giteshk pushed a commit to giteshk-org/apigee-client-php that referenced this pull request Mar 24, 2023
giteshk added a commit that referenced this pull request Mar 24, 2023
* Support assigning API products to Apps on creation (#248)

* Bump version to 2.0.19 and change log (#250)

---------

Co-authored-by: phdhiren <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support assinging API products to Apps on creation
3 participants