Bug on ranges using "last" and "previous" by mnapoli · Pull Request #7057 · matomo-org/matomo · GitHub
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.

Sign up for GitHub

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

Jump to bottom

Bug on ranges using "last" and "previous" #7057

Merged
merged 4 commits into from
Jan 21, 2015
Merged

Bug on ranges using "last" and "previous" #7057

merged 4 commits into from
Jan 21, 2015

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Jan 20, 2015

@piwik/core-team-access-all-repositories I'm opening this PR to validate with you that this is a bug and not a feature:

\Piwik\Period\Factory::build('week', 'previous');
\Piwik\Period\Factory::build('week', 'previous1');

The first one returns a period spanning on 2 weeks, whereas the second returns the expect period of one (the previous week).

I have committed failing test cases in the branch (have a look at the diff), I haven't jumped at fixing this because maybe it's intended that way and it shouldn't be broken? (but then why?)

@mnapoli mnapoli added the Bug For errors / faults / flaws / inconsistencies etc. label Jan 20, 2015
@mnapoli mnapoli self-assigned this Jan 20, 2015
@mnapoli mnapoli added this to the Piwik 2.11.0 milestone Jan 20, 2015
@diosmosis
Copy link
Member

I don't think I've ever seen 'previous' being used w/o a number. Didn't know it wouldn't fail. I'm not completely sure, but I think it was never intended to be a feature...

@mnapoli
Copy link
Contributor Author

mnapoli commented Jan 21, 2015

OK, here is the code part that's responsible for that I think:

// last1 means only one result ; last2 means 2 results so we remove only 1 to the days/weeks/etc
$lastN--;
$lastN = abs($lastN);

If $lastN = 0, then abs(0 - 1) turn it into 1, which is the same as if $lastN = 2.

@diosmosis
Copy link
Member

So I guess the issue is previous0 is handled incorrectly? Or doesn't fail if its supposed to fail. Personally, I don't see any use to previous0 or last0.

@mnapoli
Copy link
Contributor Author

mnapoli commented Jan 21, 2015

previous makes sense, but previous0 doesn't really mean anything? Or does it? What is the expected result?

Because right now previous0 behaves like previous, which wrongly behaves like previous2.

I've pushed a fix for previous and last, but I could also fix previous0 if it makes sense.

@diosmosis
Copy link
Member

Well, like I said, I've never seen previous being used by itself; that said I doubt it would hurt if it was handled to function like previous1. So I think your bug fix is correct. I don't really care about previous0, I doubt anyone will actually use it...

@mnapoli
Copy link
Contributor Author

mnapoli commented Jan 21, 2015

You are right it doesn't hurt if previous0 and last0 work too, I've fixed them all + tests. Will merge once travis is done.

mnapoli added a commit that referenced this pull request Jan 21, 2015
@mnapoli
Merge pull request #7057 from piwik/period-range-bug
638a84a 
Bug on ranges using "last" and "previous"
@mnapoli mnapoli merged commit 638a84a into master Jan 21, 2015
@mnapoli mnapoli deleted the period-range-bug branch January 21, 2015 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mnapoli mnapoli

Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Milestone
Piwik 2.11.0
Development

Successfully merging this pull request may close these issues.
2 participants
@mnapoli @diosmosis