@mnapoli opened this issue on April 3rd 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 commented on April 3rd 2015

FYI build failed because of random failure: http://builds-artifacts.piwik.org/ui-tests.rang-disabled/11472.7/screenshot-diffs/diffviewer.html

@mgazdzik commented on April 3rd 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 commented on April 7th 2015

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

@mnapoli commented on April 7th 2015

PR updated

@tsteur commented on April 7th 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 commented on April 7th 2015

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

@mnapoli commented on April 9th 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 commented on April 17th 2015

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

@mnapoli commented on April 17th 2015

Fixed, good to merge

This issue was closed on April 20th 2015
Powered by GitHub Issue Mirror