-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
FYI build failed because of random failure: http://builds-artifacts.piwik.org/ui-tests.rang-disabled/11472.7/screenshot-diffs/diffviewer.html |
@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' |
There are config settings: [General] |
PR updated |
Does it maybe make sense to fix this in the I think the error message / exception is ok if someone manipulates that URL directly etc. Also if we use eg |
Yes thanks it definitely makes sense, I will update the PR for that solution |
…erday" period Alternate implementation of #7623
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. |
private function getDefaultDateAndPeriod($defaultDate = null) | ||
{ | ||
if (! $defaultDate) { | ||
$defaultDate = $this->getDefaultPeriodWithoutValidation($defaultDate); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
The refactoring/solution looks good to me (there is one feedback to fix and it can merged then) |
Fixed, good to merge |
When selecting a disabled period, redirect to the default "yesterday" period
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.