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

Fixed #18763 -- Added with_perm() to User manager. #7153

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@berkerpeksag
Contributor

berkerpeksag commented Aug 25, 2016

@pope1ni

This comment has been minimized.

Show comment
Hide comment
@pope1ni

pope1ni Aug 26, 2016

Contributor

Generally I think this would be useful - I've wanted to know which users have which permissions in the past. However I can think of a many other cases that would be useful:

  • Which groups have certain permissions, i.e. add GroupManager.with_perm()
  • Which users have certain permissions directly assigned, i.e. not from membership of a group.
  • I may want to know what users have permissions regardless of whether they are active.
  • I may want to to exclude superusers from the output - only show explicit assignations.

Also of note is the obj parameter which is untested. According to the documentation with_perm() should return no users if obj is not None.

Contributor

pope1ni commented Aug 26, 2016

Generally I think this would be useful - I've wanted to know which users have which permissions in the past. However I can think of a many other cases that would be useful:

  • Which groups have certain permissions, i.e. add GroupManager.with_perm()
  • Which users have certain permissions directly assigned, i.e. not from membership of a group.
  • I may want to know what users have permissions regardless of whether they are active.
  • I may want to to exclude superusers from the output - only show explicit assignations.

Also of note is the obj parameter which is untested. According to the documentation with_perm() should return no users if obj is not None.

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Sep 13, 2016

Contributor

Hi @pope1ni,

Thank you for review and suggestions.

Which groups have certain permissions, i.e. add GroupManager.with_perm()

This should probably needs to be discussed in a separate ticket.

I may want to know what users have permissions regardless of whether they are active.
I may want to to exclude superusers from the output - only show explicit assignations.

+1 for adding is_superuser and is_active parameters (or exclude_*) Tim, what do you think?

Contributor

berkerpeksag commented Sep 13, 2016

Hi @pope1ni,

Thank you for review and suggestions.

Which groups have certain permissions, i.e. add GroupManager.with_perm()

This should probably needs to be discussed in a separate ticket.

I may want to know what users have permissions regardless of whether they are active.
I may want to to exclude superusers from the output - only show explicit assignations.

+1 for adding is_superuser and is_active parameters (or exclude_*) Tim, what do you think?

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Sep 13, 2016

Contributor

I think I've addressed all review comments except https://github.com/django/django/pull/7153/files#r78226234 (pending discussion) Thanks!

Contributor

berkerpeksag commented Sep 13, 2016

I think I've addressed all review comments except https://github.com/django/django/pull/7153/files#r78226234 (pending discussion) Thanks!

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Sep 22, 2016

Contributor

I've now implemented both my idea from the django-developers thread and Nick Pope's suggestions about adding is_superuser and is_active flags.

Contributor

berkerpeksag commented Sep 22, 2016

I've now implemented both my idea from the django-developers thread and Nick Pope's suggestions about adding is_superuser and is_active flags.

@rivol

This comment has been minimized.

Show comment
Hide comment
@rivol

rivol Nov 6, 2016

Contributor

Two comments regarding is_active and is_superuser parameters:

  • is_active has weird behavior IMHO. It's impossible to get ALL users with a permission, including both active and non-active users. I would skip filtering by UserModel.is_active when the value of is_active parameter is None. The valid param values would then be True/False/None. The default value could probably remain True as you only care about active users most of the time (?).
  • I would rename is_superuser to indicate that it selects whether all superusers are automatically included in results. Maybe e.g. include_superusers?
Contributor

rivol commented Nov 6, 2016

Two comments regarding is_active and is_superuser parameters:

  • is_active has weird behavior IMHO. It's impossible to get ALL users with a permission, including both active and non-active users. I would skip filtering by UserModel.is_active when the value of is_active parameter is None. The valid param values would then be True/False/None. The default value could probably remain True as you only care about active users most of the time (?).
  • I would rename is_superuser to indicate that it selects whether all superusers are automatically included in results. Maybe e.g. include_superusers?
@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Dec 30, 2016

Member

Closing due to inactivity.

Member

timgraham commented Dec 30, 2016

Closing due to inactivity.

@timgraham timgraham closed this Dec 30, 2016

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Dec 30, 2016

Contributor

I'm not sure I understand why you closed it. I didn't get any feedback or review comments from you or someone else on the Django team since September. If you think Rovi's suggestions should be addressed, you should have left a comment (for example "did you see Rovi's suggestions?" or "do you have time to address the suggestions Rovi made?") before prematurely closing the pull request.

Contributor

berkerpeksag commented Dec 30, 2016

I'm not sure I understand why you closed it. I didn't get any feedback or review comments from you or someone else on the Django team since September. If you think Rovi's suggestions should be addressed, you should have left a comment (for example "did you see Rovi's suggestions?" or "do you have time to address the suggestions Rovi made?") before prematurely closing the pull request.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Dec 30, 2016

Member

Oh, it's a misunderstanding that a team member has to validate feedback from someone else. You two could at least discuss among yourselves and come to a consensus. I guess it would be better to discuss it on the mailing list thread rather than here. Anyway, I had marked the ticket as "patch needs improvement" given the unresolved comment, so it wasn't going anywhere in the current state.

Member

timgraham commented Dec 30, 2016

Oh, it's a misunderstanding that a team member has to validate feedback from someone else. You two could at least discuss among yourselves and come to a consensus. I guess it would be better to discuss it on the mailing list thread rather than here. Anyway, I had marked the ticket as "patch needs improvement" given the unresolved comment, so it wasn't going anywhere in the current state.

@timgraham timgraham reopened this Dec 30, 2016

User.objects.with_perm(perm)
def test_with_perm_user_permissions(self):
six.assertCountEqual(

This comment has been minimized.

@timgraham

timgraham Dec 30, 2016

Member

You can use self.assertCountEqual as of b5f0b34.

@timgraham

timgraham Dec 30, 2016

Member

You can use self.assertCountEqual as of b5f0b34.

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Jan 5, 2017

Contributor

Well, from a contributor's point of view, it doesn't matter whether we (the contributors) have reached a consensus on our end or not if it doesn't get accepted by the core developers. So what I was trying to say is that at some point it would be nice to know whether we are on the same page or not.

Anyway, thanks for reopening it! I will try to address all review comments this week.

Contributor

berkerpeksag commented Jan 5, 2017

Well, from a contributor's point of view, it doesn't matter whether we (the contributors) have reached a consensus on our end or not if it doesn't get accepted by the core developers. So what I was trying to say is that at some point it would be nice to know whether we are on the same page or not.

Anyway, thanks for reopening it! I will try to address all review comments this week.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jan 5, 2017

Member

Unfortunately, it's impossible for a team member to give feedback on every issue. The lack of feedback on the mailing list is concerning (maybe there's little demand for this) but If two people come to a consensus, that's a good start.

Member

timgraham commented Jan 5, 2017

Unfortunately, it's impossible for a team member to give feedback on every issue. The lack of feedback on the mailing list is concerning (maybe there's little demand for this) but If two people come to a consensus, that's a good start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment