Review/new default permissions #222

Closed
wants to merge 4 commits into
from

Projects

None yet

4 participants

@colinnewell
Contributor

These are the changes to the default permissions for new modules added to a distribution. The code for x_authority has not been removed, but the behaviour has changed to the new copy all comaintainers behaviour. I could probably do something to retain that behaviour, but I'm not sure if that's really desirable.

I'm wondering if I need to test any more scenarios.

/cc @rjbs

colinnewell added some commits Apr 24, 2016
@colinnewell colinnewell Add failing tests for new permissions behaviour.
The plan is to make the new permissions for a new module
in an existing distribution to default to the permissions
of the module with the name of the distribution.  These
tests check for that, but fail in a TODO block until the
feature is implemented.
a9da061
@colinnewell colinnewell Added tiny note about possible failure situation ed4ec2a
@colinnewell colinnewell Ensure first come and comaint come from main package on new packages.
This changes the behaviour of new packages permissions so that rather
than giving the uploader first come, it looks to the main package
for the permissions to use.  It copies both the first come and
co maintainer users to the new package.

This should mean that for all new modules uploaded permissions should
remain fairly consistent.
d943f56
@colinnewell colinnewell Codified the actual behaviour of the permissions in the tests.
Note that x_authority's behaviour has changed with these changes.
If we want to keep that behaviour we can alter the tests back
to having just the one comaint (note reverting this commit won't
be quite correct) and change the code to prevent the copying
for x_authority modules.
bbf2d70
@rjbs
Collaborator
rjbs commented Apr 26, 2016

Thanks! I will try to review this in the next day or two.

@rjbs
Collaborator
rjbs commented May 23, 2016

rjbs commented 27 days ago
Thanks! I will try to review this in the next day or two.

I'm the worst. Reviewing now.

@rjbs rjbs commented on the diff May 23, 2016
lib/PAUSE/package.pm
FROM mods
- WHERE modid = ?", undef, $package);
- if ($sth_mods) { # make sure we regard the owner as the owner
+ WHERE modid = ?
+ ";
+ my @args = ($package);
+ unless(lc($main_package) eq lc($package)) {
+ $sql .= q{
+ UNION
+ SELECT userid
+ FROM perms
+ WHERE LOWER(package) = LOWER(?)
@rjbs
rjbs May 23, 2016 Collaborator

Not a big deal, but: this is MySQL, so this = is already case-insensitive, right? If we're going to use an explicit LOWER on both sides (which seems fine) we should probably also add it to the WHERE modid = ? above, too.

@karenetheridge
karenetheridge May 23, 2016 Contributor

Case sensitivity is controlled by collation settings, which can be set globally, at the database level and at the table level. The default might be case-insensitive, but one would need access to the database to be sure.

@colinnewell
colinnewell May 23, 2016 Contributor

I tried to be consistent with the existing code with regards to the case sensitivity. Plus don't the tests use SQLite so it needs to work in a DB server independent manner?

No lc on the modid was a case of not touching existing code any more than I needed to. I didn't even consider touching that query.

I rather suspect that that whole bit will be chopped out as I think it's related to that 03modules thing that's effectively been removed. Touching it further is probably a waste of time.

@rjbs
Collaborator
rjbs commented May 23, 2016

I think this looks good!

It took me a bit longer to remember just what every method was meant to do, but once I did, I believe it is correct. I think we should probably merge this and add an item to the PAUSE news.

@andk
Owner
andk commented Jun 7, 2016

Thanks, applied and deployed to live just now. paused restarted.

@andk andk closed this Jun 7, 2016
@karenetheridge karenetheridge added a commit to karenetheridge/Dist-Zilla-PluginBundle-Author-ETHER that referenced this pull request Jun 12, 2016
@karenetheridge karenetheridge remove [AuthorityFromModule]
PAUSE now does this automatically -- added in
andk/pause#222
7c1193c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment