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

When selecting a disabled period, redirect to the default "yesterday" period #7623

Merged
merged 4 commits into from Apr 20, 2015

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Apr 3, 2015

Fixes #7615

I redirected to hardcoded "yesterday" because I don't want to overthink it, but if you think it's a bad idea let me know.

@mnapoli mnapoli added Bug For errors / faults / flaws / inconsistencies etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Apr 3, 2015
@mnapoli mnapoli added this to the Piwik 2.13.0 milestone Apr 3, 2015
@mnapoli
Copy link
Contributor Author

mnapoli commented Apr 3, 2015

@mgazdzik
Copy link
Contributor

mgazdzik commented Apr 3, 2015

@mnapoli - maybe it could be a config setting defaulting to yesterday? I'm preety much confident that there will be a case at some point of future when somebody will prefer to set different fallback than 'yesterday'

@mattab
Copy link
Member

mattab commented Apr 7, 2015

There are config settings: [General] default_day and default_period so let's re-use those

@mnapoli
Copy link
Contributor Author

mnapoli commented Apr 7, 2015

PR updated

@tsteur
Copy link
Member

tsteur commented Apr 7, 2015

Does it maybe make sense to fix this in the getDefaultPeriod and getDefaultDate method itself? https://github.com/piwik/piwik/blob/2.13.0-b1/plugins/UsersManager/UserPreferences.php#L75-L120

I think the error message / exception is ok if someone manipulates that URL directly etc. Also if we use eg getDefaultPeriod somewhere else we would otherwise have this problem again. It would maybe also make sure to show correct value in the "User Settings" UI (eg yesterday instead of "last 7" or instead of no preselected value, haven't actually tested it)

@mnapoli
Copy link
Contributor Author

mnapoli commented Apr 7, 2015

Yes thanks it definitely makes sense, I will update the PR for that solution

@mnapoli
Copy link
Contributor Author

mnapoli commented Apr 9, 2015

I have updated the PR with an alternate solution that prevents UserPreferences to return a disallowed period or date. I have also restored the exception.

@mattab mattab added the Needs Review PRs that need a code review label Apr 10, 2015
private function getDefaultDateAndPeriod($defaultDate = null)
{
if (! $defaultDate) {
$defaultDate = $this->getDefaultPeriodWithoutValidation($defaultDate);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getDefaultPeriodWithoutValidation should be getDefaultDateWithoutValidation

edit: add unit test to catch this case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reproduced with tests and fixed

@mattab
Copy link
Member

mattab commented Apr 17, 2015

The refactoring/solution looks good to me (there is one feedback to fix and it can merged then)

@mnapoli
Copy link
Contributor Author

mnapoli commented Apr 17, 2015

Fixed, good to merge

mattab pushed a commit that referenced this pull request Apr 20, 2015
When selecting a disabled period, redirect to the default "yesterday" period
@mattab mattab merged commit 8467c61 into master Apr 20, 2015
@mnapoli mnapoli deleted the rang-disabled branch April 20, 2015 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants