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

Enable Logs page #9457

Merged
merged 5 commits into from Aug 17, 2018

Conversation

Projects
None yet
7 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Aug 16, 2018

Questions Answers
Branch? develop
Description? Search feature was validated previously, I just forgot to enable again Logs page as we were improving the Grid system. Now it's time to migrate it for good, don't you think?
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
How to test? Access Configure > Advanced Parameters > Logs

This change is Reviewable

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Aug 16, 2018

Member

I see that you drop several files about Logs, but you keep the controller?

Member

Quetzacoalt91 commented Aug 16, 2018

I see that you drop several files about Logs, but you keep the controller?

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 16, 2018

Member

@mickaelandrieu you can remove FilterLogsByAttributeType and rely on form that is built by grid.

Member

sarjon commented Aug 16, 2018

@mickaelandrieu you can remove FilterLogsByAttributeType and rely on form that is built by grid.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 16, 2018

Contributor

@sarjon done! We really need to update docs :/ there is too much things to remember for my goldfish brain !

Contributor

mickaelandrieu commented Aug 16, 2018

@sarjon done! We really need to update docs :/ there is too much things to remember for my goldfish brain !

@mickaelandrieu mickaelandrieu referenced this pull request Aug 16, 2018

Closed

Roadmap migration (1.7.5) #10

26 of 40 tasks complete

@marionf marionf self-assigned this Aug 16, 2018

{% if grid.actions.grid|length > 0 %}
{% set girdActionsButtonId = grid.id-grid ~ '-actions-button' %}
{% set gridActionsButtonId = grid.id ~ '-grid-actions-button' %}

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 16, 2018

Contributor

funny thing: I'll be able to detect this issue on Survival tests only. In dev mode, we should be allowed to ask Twig to be really strict...

@mickaelandrieu

mickaelandrieu Aug 16, 2018

Contributor

funny thing: I'll be able to detect this issue on Survival tests only. In dev mode, we should be allowed to ask Twig to be really strict...

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 16, 2018

Contributor

ping @Quetzacoalt91, this needs an extra review as I've found and fixed an error when I've uncommented my "survival" test 💪

Contributor

mickaelandrieu commented Aug 16, 2018

ping @Quetzacoalt91, this needs an extra review as I've found and fixed an error when I've uncommented my "survival" test 💪

@PierreRambaud

Useless EOL but 👍

@marionf marionf removed their assignment Aug 17, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Aug 17, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 17, 2018

Contributor

I can't explain to myself why this is not working in PHP 5.6, any idea?

Contributor

mickaelandrieu commented Aug 17, 2018

I can't explain to myself why this is not working in PHP 5.6, any idea?

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 17, 2018

Contributor

@mickaelandrieu I will try to have a check on it ;)

Contributor

PierreRambaud commented Aug 17, 2018

@mickaelandrieu I will try to have a check on it ;)

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 17, 2018

Contributor

Hey @mickaelandrieu There is a notice:

Static function PrestaShop\PrestaShop\Core\Search\Filters::getDefaults() should not be abstract

That's why test failed ;)

Contributor

PierreRambaud commented Aug 17, 2018

Hey @mickaelandrieu There is a notice:

Static function PrestaShop\PrestaShop\Core\Search\Filters::getDefaults() should not be abstract

That's why test failed ;)

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Aug 17, 2018

Member

@mickaelandrieu

The error is the following:

PHPUnit_Framework_Error_Notice

Static function PrestaShop\PrestaShop\Core\Search\Filters::getDefaults() should not be abstract

It then throws the undefined class LogsFilter.

It seems PHP does not allow abstract static methods on strict mode. I have to replace the Filters::getDefaults method with the following:

    /**
     * @return array Define the default filters configuration
     */
    public static function getDefaults()
    {
        return [];
    }
Member

Quetzacoalt91 commented Aug 17, 2018

@mickaelandrieu

The error is the following:

PHPUnit_Framework_Error_Notice

Static function PrestaShop\PrestaShop\Core\Search\Filters::getDefaults() should not be abstract

It then throws the undefined class LogsFilter.

It seems PHP does not allow abstract static methods on strict mode. I have to replace the Filters::getDefaults method with the following:

    /**
     * @return array Define the default filters configuration
     */
    public static function getDefaults()
    {
        return [];
    }
@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 17, 2018

Contributor

Wow: sneaky... well this is a proof of how cool is it to have - at least - survival tests: easy to write and so many gains (and so much pain :p) already!

Thanks for the stack trace 👍
cool

Contributor

mickaelandrieu commented Aug 17, 2018

Wow: sneaky... well this is a proof of how cool is it to have - at least - survival tests: easy to write and so many gains (and so much pain :p) already!

Thanks for the stack trace 👍
cool

@marionf marionf assigned marionf and unassigned marionf Aug 17, 2018

@marionf marionf added the QA ✔️ label Aug 17, 2018

@marionf marionf removed the waiting for QA label Aug 17, 2018

@PierreRambaud PierreRambaud merged commit 8cf4779 into PrestaShop:develop Aug 17, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PierreRambaud PierreRambaud deleted the mickaelandrieu:enable/logs branch Aug 17, 2018

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud
Contributor

PierreRambaud commented Aug 17, 2018

Thanks @mickaelandrieu ;)

@PierreRambaud PierreRambaud added this to the 1.7.5.0 milestone Aug 23, 2018

@mickaelandrieu mickaelandrieu referenced this pull request Sep 5, 2018

Open

Symfony migration roadmap for 1.7.5 #10301

27 of 40 tasks complete

@matks matks added the migration label Sep 18, 2018

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