Permission to "approve other suggestions only"

Good feature idea. I'll look into implementing this later if nobody beats me to it. It needs some thought on the integration with Organic Groups as well, since it only provides one level of permissions (either you are a group member or an admin).

This is quite easy to game though with two user accounts. Also I assume you expect people to play fair :)

This is very important feature that makes big troubles... i hope you find some time to do this very soon.

This is a feature request, I think a good one. Maybe somebody will pick it up? I might but fixing bugs seems to have higher priority

The problem with "Approve other suggestions only" is, that the code gets uglier, because user_access("Approve other suggestions only") return TRUE for the user1, which is kind of unlogical.

Here is a patch which does only the check with two user access(suggester and approver). It needs more testing

A couple of remarks:

• We should use lower case permission names
• We should drop submit translations and approve suggestions in favor of the more granular submit suggestions, approve own suggestions and approve suggestions. That way, we could also create ‘reviewer only’ accounts.

Agreed with @kkaefer!

I also like the idea
I will have time this afternoon to do it.

Here is a more complete patch I just received from @kkaefer yesterday. This is broken out of the bigger patch at #563128: Rework the translation UI. I did a deep reading of this patch, and found the following issues:

- minor: l10n_community_string_suggestions() could use cleaner variable assingment to use if ($perm) instead of using ternary operators with empty second values
- l10n_community_import_form() checks for L10N_PERM_MODERATE_OWN, but people can use that form to submit .po files in the name of the group (which is then not attributed to their name but to the "multiple contributors" user); in that case, their act of clicking this button would require the L10N_PERM_MODERATE_OTHERS permission. So I'd say we should display that checkbox if either moderation permissions are given and then either have a description and/or JS to disable/enable that checkbox depending on the permission levels and handle it on the server side as well accordingly.
- I'm seeing L10N_PERM_STABILIZE_OTHERS and L10N_PERM_STABILIZE_OWN in the patch, which are then not handled (given that this patch is not about supporting that feature); I'd suggest to remove this, and would say that adding them later as bigger numbers should not confuse the listing (string permissions will not be exactly close by in terms of bits, but that is fine IMHO).

I'll look into these and the test failures that Konstantin mentioned.

The patch did not apply anymore due to changes to the moderation page filter handling and the import screen, as well as import tests added. Fixed where it did not apply and rerolled as a first step.

Attached patch fixes some issues:

- implemented conditional wrapper instead of ternaries in l10n_community_string_suggestions()
- found out that l10n_community_string_ajax_suggestion() was only accepting approval/decline actions, if the user had both permission to approve own and approve other suggestions; needed to use the same logic as in the builder function
- fixed permission check on top of l10n_community_import_page(), since checking for IMPORT permission is late enough here; we should use the moderation permissions instead
- removed the stabilization constants
- reverted to the old permission checking code in l10n_community_block_help(); I could not figure out why did you try to use NULL as a language code to ask for permissions, that should theoretically never happen
- added detailed comments on l10n_community_moderation_form() on who's responsibility is to check permissions ahead
- fixed l10n_community_pick_go_submit() to use the suggest permission, since import is not required when jumping to the edit or view page (previous patch used the more generic contribute permission)
- added a bunch of @todo fixme comments with explanation on what still needs to be done; mostly description changes about what a user has permission to do

Running this through our testsuite in the module says 934 passes, 4 fails, and 0 exceptions
Looks like those 4 failures are due to individual approve and decline actions failing.

No good idea to use single quotes in permission names... remember the still unfixed bug #540016: system_update_6034() will fail if permission have single quote

This review is powered by Dreditor.

subscribe

@hass: any better suggestions for naming?

Updated patch now passes tests. The ajax submission callback handler was not using $suggestion->language but $langcode, which was nonexistent in that context.

Fixed use of l10n_community_get_permission() to use $perm consistently. One place used $permissions for example, which is elsewhere used for the whole array of permissions for all users and languages.

Also found an issue with permissions for the moderate page, which requires review access to be used, but in fact, it would work with either moderation access right. Added a todo for now.

You have said in http://drupal.org/node/540016#comment-1927142

Also, we assume permission names will not have single quotes in them.

:-)

@hass: again, any better suggestions for naming?

Not really good, but it need to be changed as it's unexpected. But You have thankfully solved the source of this issue in D7 - so this single quotes can go into the permission UI names / descriptions in D7. But this is future music...

I suggest "moderate suggestions from others" or "moderate suggestions by others" for permission name. (Not sure which one's best, but both should be reasonably clear.)

Used "moderate suggestions from others" in the updated attached patch. Still needs to fix the @todo's.