Skip to content

Fix admin on group/user permissions #130

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

Closed
wants to merge 5 commits into from

Conversation

BrendaH
Copy link
Contributor

@BrendaH BrendaH commented Jul 10, 2018

Addresses issue #126

Add a setting to make the used widget for selecting user/group for permissions customizeable and take into account the right to actually change user/group-permissions on forums.

No tests yet

NOTE: There are no tests yet for this code because I have not been able to get a test environment running on my windows machine and writing tests without being able to run them seemed like a bad idea. So if anybody is able to do this: please feel free.
I did of course run this code locally and thereby tested it and will probably try (again..) in the future to get a test environment running.

Meanwhile: feel free to test and comment or suggest improvements.

My use case

I'm already using the new setting locally like this:

settings.py:

MACHINA_FORUM_PERMISSIONS_USER_SELECT_WIDGET_FUNC = 'pe.widgets.machina_select_user_widget'
MACHINA_FORUM_PERMISSIONS_GROUP_SELECT_WIDGET_FUNC = 'pe.widgets.machina_select_group_widget'

pe.widgets.py:

def machina_select_user_widget(field, admin_site, **kwargs):
	"""
	Differs from the function below in that we want to display the whole name
	and not just the username which is the default string representation.
	"""
	# For a normal select:
	qset = field.related_model.objects.filter(is_active=True).order_by('first_name', 'last_name')
	choices = [(None, _('----'))]
	for obj in qset:
		choices.append((obj.id, obj.get_full_name()))
	return Select(choices=choices)

def machina_select_group_widget(field, admin_site, **kwargs):
	"""
	Differs from the function above this one in that it does not need to do
	difficult things to the string representation of the object.
	"""
	# For a normal select:
	qset = field.related_model.objects.all()
	choices = [(None, _('----'))]
	for obj in qset:
		choices.append((obj.id, str(obj)))
	return Select(choices=choices)

For me this creates a pulldown (select) for both users and groups without the need to have change permissions on groups/users. And I can even display the full names of the users instead of the default user name that a Select would normally show.

I have about 230 users in my project of which 170 are active, this gives me no noticeable overhead.

…n forums

References ellmetha#126.

The forms to select user or group to edit it's permissions (on a forum)
are now only built if you have the right (granted via the auth system) to change
the user or group permissions on forums.

Possible errors (not choosing ID/anonymous user but still submitting) are
displayed above the form that was actually submitted.

In the template the html is only made for the forms that need to be shown. And
for the group permission form a section for errors is added.

NOTE: the user still needs permission to change (i.e. see) users and groups to
actually be able to use these forms (the selection of the id), this has not
changed.
…permissions

References ellmetha#126

REASON:
Before this commit the widget that is used to select a user or group to alter
its permissions is Django's ForeignKeyRawIdWidget. This widget requires that
the user has change right on the group and user to actually see the list to
select the target from.
Also, the widget presents the user with an integer field which might be
perceived as not completely user friendly.
This approach was taken to reduce the load associated with presenting a select
box of targets.

PROPOSITION:
Now the developer can decide what widget to use to select user or group by
writing a callable and putting the dotted path to it inside the new settings.
This may take away the need to have to grant change right on users/groups.
This also provides the possibility to write a callable that influences the
presentation (for example full name instead of user name for user Select).
When the setting is not overwritten in own settings file the default is still
used.

CHANGES:
Added setting for both group and user (though default these both use the same
callable).
Added documentation for these settings.
Created callable to link to in setting, this callable just returns the
ForeignKeyRawIdWidget like in the former behavior.
Add helperfunction _get_setting_callable to get an actual callable from the
dotted path in the setting.
Use the new settings in PickUserForm and PickGroupForm.
Since the content of _get_setting_callable is very much the same (one
improvement on the AssertionError) as the content of _get_markup_widget, use
this new function in _get_markup_widget.
Behavior of _get_markup_widget is not changed by this.
editpermissions_index_view in admin.py checks which form is posted by checking
the presence of '_select_user' or '_select_group' (save button) in the post and
will not return a HttpResponseObject if the form was not posted by 'use' of a
button.
In the test we now simulate the use of the corresponding button by passing on
the parameter manually.
Test if we get errors set when group/user/anonymous user is missing when
submitting the corresponding form in the editpermission view.
@BrendaH
Copy link
Contributor Author

BrendaH commented Aug 16, 2018

I was finally able to put up a test environment* and write tests for this improvement.

If there's an objection against these changes, please let me know. Else I'd like to see this PR merged. :-)

[*] Trouble on my windows machine was that colorama was required but not installed because the Pipfile.lock does not list it (Linux doesn't need it) and any errors about it were eaten by pluggy. (pytest-dev/pluggy#174)

Copy link
Owner

@ellmetha ellmetha left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to submit this PR.

While I agree that the issue described in #126 should be addressed (the fact that user/group permissions related to forum_permission should be taken into account in the forum permissions page), I do not agree with the general approach of this PR. IMHO django-machina already provides a convenient way to lazy load classes or functions (through the use of the get_class helper). I don't see the need to create a new helper such as _get_setting_callable when you can make use of get_class to dynamically import the form classes used in the permission view. Of course these forms are now in the machina.forum.admin module but you could create a new machina.forum.forms module, put the considered forms in there and then import them in the admin module using get_class. By doing so we wouldn't have to add two new settings and you could still easily change the widgets of the permissions forms by making use of the machina's dynamic class-loading system.

What do you think?

@BrendaH
Copy link
Contributor Author

BrendaH commented Aug 20, 2018

Might be a good idea. Would've liked to know this earlier though, since at this moment I have lots of other work to do and will not be able for a while to work on this while earlier on I put quite some time in getting the tests (and test environment) for these changes going. Time better spent in redoing the whole thing the Machina-way, had I known. :-(

So yeah, I understand not merging this PR, but for now I can not work on it.
Anybody else is free to have a go at it. :-)

@ellmetha
Copy link
Owner

Closing. 660b34a introduces a change that allows to rely on the class loading mechanism to easily override forum permission forms (including fields and widgets).

@ellmetha ellmetha closed this Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants