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

Fixed filtering for date in Back Office / Logs #16883

Merged
merged 3 commits into from Jan 10, 2020

Conversation

@PrestaworksNiklas
Copy link
Contributor

PrestaworksNiklas commented Dec 20, 2019

Fix for filter by date.

Questions Answers
Branch? develop
Description? Fixed filtering for date in Back Office / Logs
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes issue #16540
How to test? See below

How to test ?

Try to filter logs for 1 day : fill in the date filters with the same date (example: 2019-12-30 for both "from" and "to"). You should see the logs for date 2019-12-30 but instead got empty results. The PR fixes it.


This change is Reviewable

Fix for filter by date.
@PrestaworksNiklas PrestaworksNiklas requested a review from PrestaShop/prestashop-core-developers as a code owner Dec 20, 2019
@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Dec 20, 2019

Hello @PrestaworksNiklas!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@PrestaworksNiklas PrestaworksNiklas changed the title Update LogRepository.php Fixed filtering for date in Back Office / Logs Dec 20, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 20, 2019

Hello @PrestaworksNiklas and thank you for the PR !
The logic you added with hours is good 😉, but why do you remove the following check:

if (!empty($filterValue['from']) &&
                    !empty($filterValue['to'])
                )

This check makes sure a real value is provided from 'from' and 'to', so they seem important ?

$qb->setParameter('date_to', sprintf('%s 23:59:59', $filterValue['to']));

if (isset($filterValue['from'])) {
$qb->andWhere('l.date_add >= :date_from');

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 21, 2019

Contributor

I don't understand why you duplicate this condition from line 255.

You must choice between using:

                $qb->andWhere('l.date_add >= :date_from AND l.date_add <= :date_to');
                $qb->setParameter('date_from', sprintf('%s 0:0:0', $filterValue['from']));
                $qb->setParameter('date_to', sprintf('%s 23:59:59', $filterValue['to']));

and

                if (isset($filterValue['from'])) {
                    $qb->andWhere('l.date_add >= :date_from');
                    $qb->setParameter('date_from', sprintf('%s 0:0:0', $filterValue['from']));
                }

                if (isset($filterValue['to'])) {
                    $qb->andWhere('l.date_add <= :date_to');
                    $qb->setParameter('date_to', sprintf('%s 23:59:59', $filterValue['to']));
                }

But you can't use both, it's useless 🤔

This comment has been minimized.

Copy link
@PrestaworksNiklas

PrestaworksNiklas Dec 23, 2019

Author Contributor

I have done a new commit now. The reason I inserted the condition twice was because I found the same condition inside the file CustomerQueryBuilder.php, in src/Core/Grid/Query. See here

This comment has been minimized.

Copy link
@matks

matks Dec 30, 2019

Contributor

I have done a new commit now. The reason I inserted the condition twice was because I found the same condition inside the file CustomerQueryBuilder.php, in src/Core/Grid/Query. See here

It was good to reuse another part of code but I think this part of code actually is buggy 😄 I'll check it

Copy link
Contributor

PierreRambaud left a comment

See comment :)

PrestaworksNiklas and others added 2 commits Dec 23, 2019
Fix changes addressed in comment
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 30, 2019

In order not to bother @PrestaworksNiklas anymore I fixed myself the small issues 😉let's go for QA now

@matks matks dismissed PierreRambaud’s stale review Dec 30, 2019

Requested change applied

@matks
matks approved these changes Dec 30, 2019
@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Jan 10, 2020
@sarahdib sarahdib added this to the 1.7.7.0 milestone Jan 10, 2020
@Progi1984 Progi1984 merged commit 28dc70a into PrestaShop:develop Jan 10, 2020
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Jan 10, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.