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

Keep showcase cards closed #12111

Merged
merged 13 commits into from Jan 17, 2019

Conversation

Projects
None yet
8 participants
@eternoendless
Copy link
Member

eternoendless commented Jan 10, 2019

Questions Answers
Branch? 1.7.5.x
Description? The showcase card in SEO & URLs page didn't stay closed when you reloaded the page. Now it does :)
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #12047
How to test? See issue

Notable things to keep track of

New setting

This Pull Request stores a new setting in table ps_configuration: PS_SHOWCASECARD_SEO_URLS_CLOSED

Value Meaning
(not present) Not closed
0 Not closed
1 Card has been closed

Generic handling for all showcase cards

This PR lays a foundation to make this feature work with any number of showcase cards. To support a new one, you need to edit ShowcaseCard and add a new ID, then edit ConfigurationMap to map the created ID with a configuration to be stored in ps_configuration.

Also, you may have noticed that even though the commands and queries receive $employeeId, handlers don't make use of it. This is to support per-employee tracking of showcase cards in the future (right now, if you close the showcase card, you are closing it for everyone).


This change is Reviewable

public function handle(CloseShowcaseCardCommand $command)
{
$configurationName = $this->configurationMap->getConfigurationNameForClosedStatus($command->getShowcaseCard());
$this->configuration->set($configurationName, '1');

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jan 10, 2019

Member

Why not sending an integer as param?

This comment has been minimized.

@eternoendless

eternoendless Jan 10, 2019

Author Member

For consistency's sake. Since values are actually stored as strings in ps_configuration, it will come back as a string when reading it from the DB.

If I set it as an int, then the return type for $configuration->get() would be different depending on whether it was read from memory or from DB.

public function handle(GetShowcaseCardIsClosed $query)
{
$configurationName = $this->configurationMap->getConfigurationNameForClosedStatus($query->getShowcaseCard());
return (bool) $this->configuration->get($configurationName);

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jan 10, 2019

Member

getBoolean(...) should be available, even if I just figured out they are not declared in the interface. :/

This comment has been minimized.

@eternoendless

eternoendless Jan 10, 2019

Author Member

I know, same for the default value... but since it's not in the interface, I can't really use it 😕

This comment has been minimized.

@sarjon

sarjon Jan 11, 2019

Member

there is PR to fix it #9193, which also points out some other issues with current Configuration adapter. @eternoendless do you think it should be (if at all should be) part of 1.7.6?

@eternoendless eternoendless force-pushed the eternoendless:close-showcase branch from a1cfc1e to 1dd9535 Jan 10, 2019

@@ -0,0 +1,83 @@
<?php

This comment has been minimized.

@sarjon

sarjon Jan 11, 2019

Member

i guess cs fixer is missing? other files as well.

This comment has been minimized.

@eternoendless

eternoendless Jan 11, 2019

Author Member

It's weird, I launched it several times but it's not fixing everything every time...

Show resolved Hide resolved src/Core/Domain/ShowcaseCard/Command/CloseShowcaseCardCommand.php
/**
* Saves the showcase card status to keep it closed
*/
class CloseShowcaseCardHandler implements CloseShowcaseCardHandlerInterface

This comment has been minimized.

@sarjon

sarjon Jan 11, 2019

Member

shouldnt it be final?

This comment has been minimized.

@eternoendless

eternoendless Jan 11, 2019

Author Member

Done

Show resolved Hide resolved src/Core/Domain/ShowcaseCard/CommandHandler/CloseShowcaseCardHandler.php
Show resolved Hide resolved src/Core/Domain/ShowcaseCard/ConfigurationMap.php Outdated
Show resolved Hide resolved ...Core/Domain/ShowcaseCard/QueryHandler/GetShowcaseCardIsClosedHandler.php Outdated
Show resolved Hide resolved ...Core/Domain/ShowcaseCard/QueryHandler/GetShowcaseCardIsClosedHandler.php Outdated
Show resolved Hide resolved ...in/ShowcaseCard/QueryHandler/GetShowcaseCardIsClosedHandlerInterface.php
Show resolved Hide resolved src/Core/Domain/ShowcaseCard/ValueObject/ShowcaseCard.php Outdated
Show resolved Hide resolved ...aShopBundle/Controller/Admin/Configure/ShopParameters/MetaController.php Outdated

@eternoendless eternoendless force-pushed the eternoendless:close-showcase branch from 1dd9535 to 31dd33d Jan 11, 2019

@eternoendless

This comment has been minimized.

Copy link
Member Author

eternoendless commented Jan 11, 2019

I have no idea why php-cs-fixer is failing now.

@mbadrani mbadrani self-assigned this Jan 17, 2019

@eternoendless eternoendless force-pushed the eternoendless:close-showcase branch from 2b0ba7a to eb6fa9f Jan 17, 2019

@eternoendless

This comment has been minimized.

Copy link
Member Author

eternoendless commented Jan 17, 2019

There was a weird merge conflict in the built assets, I had to rebuild them. Nothing else changed.

@mbadrani mbadrani added QA ✔️ and removed waiting for QA labels Jan 17, 2019

@mbadrani

This comment has been minimized.

Copy link
Contributor

mbadrani commented Jan 17, 2019

@eternoendless checked in my side

@mbadrani mbadrani removed their assignment Jan 17, 2019

@PierreRambaud PierreRambaud merged commit 583bff9 into PrestaShop:1.7.5.x Jan 17, 2019

1 check passed

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

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 17, 2019

@eternoendless eternoendless deleted the eternoendless:close-showcase branch Jan 17, 2019

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Jan 31, 2019

@eternoendless In multistore configuration, if I close the card on SEO&Urls page and refresh the page, it reappears
Issue: #12390

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