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

Jump to bottom

Set a user preference only if user has actually at least view permission to one site #7841

Merged
merged 3 commits into from May 6, 2015
Merged

Set a user preference only if user has actually at least view permission to one site #7841

merged 3 commits into from May 6, 2015

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented May 5, 2015

refs #7839

Background: There is a task that sets the default preference "Report To Load By Default" for each user if none is configured yet. In my Piwik I had many entries in the option table (this is where those values are stored for each user) with an empty value '' because many users do not have access to any site. Instead we should only set the default preference when there is actually a site.

Otherwise following scenario possible I think: User has stored a default preference for defaultReport ='' because at time, when the task ran, the user did not have access to any site. Now a user gets permission to a site but default preference is still '' meaning because of this check it will return '' as preference and not the default preference: https://github.com/piwik/piwik/blob/2.13.0-rc3/plugins/UsersManager/API.php#L106

@tsteur tsteur changed the title Set a user preference only if there is actually a site Set a user preference only if user has actually at least view permission to one site May 5, 2015
@tsteur tsteur added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label May 5, 2015
tsteur
tsteur reviewed May 5, 2015
View reviewed changes
core/Updates/2.13.1.php
*
*/

namespace Piwik\Updates;
Copy link
Member Author

Choose a reason for hiding this comment

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

I placed this update file first in UsersManager plugin since it is needed because of logic there and couldn't really decide whether to place it in that plugin or in core. Placed it in core since it is kinda nice to be able to see all update scripts in one place but I think it rather belongs to UsersManager and we should maybe start putting them into the related plugins if they belong there.

@tsteur tsteur added the Needs Review PRs that need a code review label May 5, 2015
@tsteur tsteur added this to the 2.13.1 milestone May 5, 2015
diosmosis
diosmosis reviewed May 6, 2015
View reviewed changes
core/Updates/2.13.1.php
static function getSql()
{
$optionTable = Common::prefixTable('option');
$removeEmptyDefaultReportsSql = "delete from $optionTable where option_name like '%defaultReport%' and option_value=''";
Copy link
Member

Choose a reason for hiding this comment

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

Should be "delete from $optionTable", otherwise will fail on MySQL if there's no table prefix.

@diosmosis
Copy link
Member

Instead of the $optionValue !== false check, can we check !empty($optionValue)? Actually in this case I guess UserPreferences::getUserPreference() will still be an empty string. Perhaps it would be a better solution if the caller handled the case where it was an empty string?

@tsteur
Copy link
Member Author

tsteur commented May 6, 2015

The wrong ones are deleted in the update sql so it should not return an empty value anymore apart from false. Option returns false when there is no value yet. In theory we could do !empty($option) as it is either 'all' or an idSite that is >= 1. If a different preferenceName is given this might not work since an empty value can be a valid value. I'd leave it there as it is for now. Oh and yes, ideally getPreferenceValue decided whether it is an acceptable value or not and returns false or an actual value.

diosmosis added a commit that referenced this pull request May 6, 2015
@diosmosis
Merge pull request #7841 from piwik/7839
c283745 
Set defaultReport to load user preference only if user has actually at least view permission to one site, so the user preference will not be an empty string.
@diosmosis diosmosis merged commit c283745 into master May 6, 2015
@diosmosis diosmosis deleted the 7839 branch May 6, 2015 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
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
Milestone
2.13.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tsteur @diosmosis