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

Add cover & menu thumbnail images deleting for category #11481

Merged
merged 12 commits into from Jan 5, 2019

Conversation

Projects
None yet
8 participants
@sarjon
Copy link
Member

sarjon commented Nov 23, 2018

Status: ready to merge after rebase

Questions Answers
Branch? develop
Description? Allows deleting cover and menu thumbnail images when editing category.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #10194
How to test? You should be able to delete images from category edit page as you were able in legacy page. admin-dev/sell/catalog/categories

This change is Reviewable

@sarjon sarjon changed the title Add cover & menu thumbnail images deleting Add cover & menu thumbnail images deleting for category Nov 23, 2018

@matks matks added the migration label Nov 26, 2018

@@ -75,6 +75,6 @@ private function setCategoryId($categoryId)
);
}
$this->categoryId = $categoryId;
$this->categoryId = (int) $categoryId;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 27, 2018

Contributor

you shouldn't do that here, instead better validate the input data 👍

@matks
Copy link
Contributor

matks left a comment

No changes except the comment from Mickael 👍

*
* new FormSubmitButton();
*/
export default class FormSubmitButton {

This comment has been minimized.

@matks

matks Nov 29, 2018

Contributor

👍 nice one !

This comment has been minimized.

@matks

matks Nov 29, 2018

Contributor

Does this mean we should go back on already migrated pages to use this component everywhere ?

This comment has been minimized.

@sarjon

sarjon Nov 30, 2018

Author Member

we could, but its not must. :)

Show resolved Hide resolved src/Core/Domain/Category/ValueObject/MenuThumbnailId.php Outdated
@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Nov 30, 2018

@matks done. :)

@matks matks dismissed their stale review Dec 5, 2018

Applied changes

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Dec 17, 2018

@matks i think i've addressed all the comments.

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Dec 18, 2018

@ntiepresta be careful, previous PR has not been tested, so we need to test the whole add/edit category page

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Dec 19, 2018

@marionf note that not all functionality of Category form is migrated yet, so its pretty hard to test. :/

@matks matks dismissed their stale review via 3bb9423 Jan 4, 2019

@matks matks force-pushed the sarjon:add-missing-category-features branch from 5a2fbf7 to 3bb9423 Jan 4, 2019

@matks

matks approved these changes Jan 4, 2019

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 4, 2019

Rebased, waiting for Travis before merge...

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 5, 2019

@matks regarding this error:

Caused by
GuzzleHttp\Exception\ConnectException: cURL error 28: Operation timed out after 5001 milliseconds with 52198 bytes received

if i understand correctly, it does actual http request? shouldnt it be mocked it tests instread? 🤔

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 5, 2019

It is a server timeout, i rerun tests for you @sarjon

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 5, 2019

Still waiting for wording before merge?

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Jan 5, 2019

Wording was confirmed already, its just @prestonBot who enjoys adding labels. -_-

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 5, 2019

Thanks @sarjon !

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 5, 2019

Thanks @PierreRambaud !

@matks matks merged commit 4ac7920 into PrestaShop:develop Jan 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PierreRambaud PierreRambaud added this to the 1.7.6.0 milestone Jan 7, 2019

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