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

Get, Update, Post and delete catalog promotions #12994

Merged
merged 5 commits into from Aug 25, 2021

Conversation

arti0090
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
License MIT

extends #12986

@arti0090 arti0090 requested a review from a team as a code owner August 23, 2021 09:40
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Aug 23, 2021
@arti0090 arti0090 changed the title Update and delete catalog promotions [WIP] Get, Update and delete catalog promotions Aug 23, 2021
@arti0090 arti0090 force-pushed the update-and-delete-catalog-promotions branch from dbf1c91 to 80231af Compare August 24, 2021 07:27
Copy link
Member

@GSadee GSadee left a comment

Choose a reason for hiding this comment

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

IMO, in this PR you should add also behat scenarios for updating and deleting catalog promotion

tests/Api/Admin/CatalogPromotionsTest.php Outdated Show resolved Hide resolved
tests/Api/Admin/CatalogPromotionsTest.php Outdated Show resolved Hide resolved
tests/Api/Admin/CatalogPromotionsTest.php Outdated Show resolved Hide resolved
tests/Api/Admin/CatalogPromotionsTest.php Outdated Show resolved Hide resolved
tests/Api/Admin/CatalogPromotionsTest.php Outdated Show resolved Hide resolved
tests/Api/Admin/CatalogPromotionsTest.php Outdated Show resolved Hide resolved
tests/Api/Admin/CatalogPromotionsTest.php Outdated Show resolved Hide resolved
tests/Api/Admin/CatalogPromotionsTest.php Outdated Show resolved Hide resolved
@arti0090 arti0090 force-pushed the update-and-delete-catalog-promotions branch from 524246b to 089e60a Compare August 24, 2021 11:50
@arti0090 arti0090 force-pushed the update-and-delete-catalog-promotions branch from b8062d5 to 5128035 Compare August 24, 2021 12:15
@arti0090 arti0090 changed the title [WIP] Get, Update and delete catalog promotions Get, Update and delete catalog promotions Aug 24, 2021
@arti0090 arti0090 changed the title Get, Update and delete catalog promotions Get, Update, Post and delete catalog promotions Aug 24, 2021
@arti0090 arti0090 force-pushed the update-and-delete-catalog-promotions branch from 5128035 to 9044d8f Compare August 24, 2021 12:30
@arti0090
Copy link
Contributor Author

Behat tests for this feature will be added in next PR 🎉

Comment on lines +52 to +54
<attribute name="normalization_context">
<attribute name="groups">admin:catalog_promotion:update</attribute>
</attribute>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<attribute name="normalization_context">
<attribute name="groups">admin:catalog_promotion:update</attribute>
</attribute>
<attribute name="normalization_context">
<attribute name="groups">admin:catalog_promotion:read</attribute>
</attribute>

@@ -30,12 +30,38 @@

<collectionOperation name="admin_post">
<attribute name="method">POST</attribute>
<attribute name="normalization_context">
<attribute name="groups">admin:catalog_promotion:create</attribute>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<attribute name="groups">admin:catalog_promotion:create</attribute>
<attribute name="groups">admin:catalog_promotion:read</attribute>


$response = $this->client->getResponse();

$this->assertResponse($response, 'admin/catalog_promotion/get_catalog_promotions_admin_response', Response::HTTP_OK);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->assertResponse($response, 'admin/catalog_promotion/get_catalog_promotions_admin_response', Response::HTTP_OK);
$this->assertResponse($response, 'admin/catalog_promotion/get_catalog_promotions_response', Response::HTTP_OK);

[],
[],
$header
);

$response = $this->client->getResponse();

$this->assertResponse($response, 'admin/get_catalog_promotions_response', Response::HTTP_OK);
$this->assertResponse($response, 'admin/catalog_promotion/get_catalog_promotion_admin_response', Response::HTTP_OK);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->assertResponse($response, 'admin/catalog_promotion/get_catalog_promotion_admin_response', Response::HTTP_OK);
$this->assertResponse($response, 'admin/catalog_promotion/get_catalog_promotion_response', Response::HTTP_OK);


$response = $this->client->getResponse();

$this->assertResponse($response, 'admin/catalog_promotion/post_catalog_promotion_admin_response', Response::HTTP_CREATED);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->assertResponse($response, 'admin/catalog_promotion/post_catalog_promotion_admin_response', Response::HTTP_CREATED);
$this->assertResponse($response, 'admin/catalog_promotion/post_catalog_promotion_response', Response::HTTP_CREATED);


$response = $this->client->getResponse();

$this->assertResponse($response, 'admin/catalog_promotion/put_catalog_promotion_admin_response', Response::HTTP_OK);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->assertResponse($response, 'admin/catalog_promotion/put_catalog_promotion_admin_response', Response::HTTP_OK);
$this->assertResponse($response, 'admin/catalog_promotion/put_catalog_promotion_response', Response::HTTP_OK);

$token = $this->logInAdminUser('api@example.com');
$authorizationHeader = self::$container->getParameter('sylius.api.authorization_header');
$header['HTTP_' . $authorizationHeader] = 'Bearer ' . $token;
return array_merge($header, self::CONTENT_TYPE_HEADER);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return array_merge($header, self::CONTENT_TYPE_HEADER);
return array_merge($header, self::CONTENT_TYPE_HEADER);

@@ -6,7 +6,7 @@
{
"@id": "\/api\/v2\/admin\/catalog-promotions\/mugs_discount",
"@type": "CatalogPromotion",
"id": @integer@,
"id": "@integer@",
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change that? IMO here and in other responses there should be:

Suggested change
"id": "@integer@",
"id": @integer@,

@GSadee GSadee merged commit cb9c140 into Sylius:master Aug 25, 2021
@GSadee
Copy link
Member

GSadee commented Aug 25, 2021

Thank you, @arti0090! 🥇

GSadee added a commit that referenced this pull request Aug 25, 2021
This PR was merged into the 1.11-dev branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | master
| Bug fix?        | no
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| License         | MIT

minor fixes of  #12994

Commits
-------

54328e9 Fixes to endpoints and contract tests of CP
GSadee added a commit that referenced this pull request Aug 26, 2021
… (arti0090)

This PR was merged into the 1.11-dev branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | master
| Bug fix?        | no
| New feature?    | yes (behat for feature)
| BC breaks?      | no
| Deprecations?   | no
| License         | MIT

Promised behats for #12994


Commits
-------

3e37138 Add delete behat with factory
c690e3b Delete covered in behat
be54912 Edit covered in behats
4e67186 Revert behat of delete
2912c4f Changing services and tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants