Create hooks to allow the implementation of Cross-PTEs #537

Closed
akirk opened this Issue Sep 9, 2016 · 14 comments

Projects

None yet

3 participants

@akirk
Contributor
akirk commented Sep 9, 2016

This is in response to the efforts to create Cross-PTEs on WordPress.org.

We are planning to achieve this with a special permission that achieves the behavior by re-using the approve permission and will need to add a filter.

@akirk akirk added the enhancement label Sep 9, 2016
@akirk akirk added this to the 2.2 milestone Sep 9, 2016
@akirk akirk self-assigned this Sep 9, 2016
@akirk akirk added a commit to akirk/GlotPress that referenced this issue Sep 9, 2016
@akirk akirk Fixes #537 by adding the filters that allow the implementation of Cro…
…ss-PTEs
4515621
@akirk akirk added a commit to akirk/GlotPress that referenced this issue Sep 9, 2016
@akirk akirk Fixes #537 by adding the filters that allow the implementation of Cro…
…ss-PTEs
59bdda4
@akirk akirk added a commit to akirk/GlotPress that referenced this issue Sep 9, 2016
@akirk akirk Fixes #537 by adding the filters that allow the implementation of Cro…
…ss-PTEs
6d12cc7
@akirk akirk added the has PR label Sep 9, 2016
@akirk akirk added a commit to akirk/GlotPress that referenced this issue Sep 9, 2016
@akirk akirk Fixes #537 by adding the filters that allow the implementation of Cro…
…ss-PTEs
b18637c
@toolstack
Contributor

Re-using an existing permssion seems wrong, why not create a new permission and use the existing hooks in GP_Permission->user_can() to override the built in behaviour?

@akirk
Contributor
akirk commented Sep 9, 2016

Well, Cross-Locale PTE is probably something very WordPress.org specific. If we introduced another permission, I'd need to add checks for that in several places. As you can see in my example plugin implementation I actually do use the permission system for a cross-pte permission.

@toolstack
Contributor

All the checks you need should be in place already (GP checks for approver rights in all the required places), I'd think it would be a simple hook at the user_can() level to chain on a second check for cross-pte as well.

Then the whole thing could be done as a plugin without any changes to GP.

@toolstack
Contributor

Something like:

class GP_Project_Approvers {
    public $id = 'gp-project-approvers';

    public function __construct() {
        add_filter( 'gp_can_user', array( $this, 'gp_can_user' ), 10, 2 );
    }

    public gp_can_user( $verdict, $args ) {
        if ( false === $verdict && 'approve' === $args['action'] && 'translation-set' === $args['object_type'] ) {
            $set = GP::$translation_set->get( $args['object_id'] );

            return GP::$permission->user_can( $args['user'], 'cross-pte', 'project', $set->project_id );

        }

        return $verdict;
    }
}
@ocean90
Member
ocean90 commented Sep 9, 2016

@toolstack The important part of this is that we also have to check permissions per translation/original, not translation sets.

@toolstack
Contributor

@ocean90 perhaps I'm missing something but based on the requirements, per translation permissions seems to be overkill.

Correct me if I'm wrong but it looks like the idea is to block a cross-PTE from overwriting and translation from a PTE or a GTE?

I guess my basic problem with the approach is that it's effectively creating a second permission system via the filters for setting the status of a translation. That seems like a hack that should have a more general solution that would work for other things as well.

Perhaps creating a second can() call for the specific translation like:

$can_approve = $this->can( 'approve', 'translation-set', $translation_set->id ) && $this->can( 'approve', 'translation', $t ); 
@ocean90
Member
ocean90 commented Sep 9, 2016

GP_Permission::current_user_can() has actually four parameters. The fourth is called $extra and can be any arbitrary data.

GP_Route::can() supports only three parameters. Should we make the fourth parameter available since it's just a wrapper for GP_Permission::current_user_can()?

$can_approve = $this->can(
    'approve',
    'translation-set',
    $translation_set->id,
   [ 'translation' => $t ]
);
@toolstack
Contributor

Making the fourth parameter available makes sense in general, but I'd still keep it as a separate call.

We're really extending the existing permission system to allow for per translation permissions, which seems like a separate logical check. That way it's easy to capture as part of the gp_user_can filter.

There could be an argument for removing the translation-set check altogether, but that might have other impacts so keeping it for backwards compatibility seems like a reasonable compromise.

@akirk
Contributor
akirk commented Sep 12, 2016

FWIW I tried to introduce a GP::$permission->current_user_can( 'approve', 'translation', $t->id, $t ) and replacing 'approve', 'translation-set' calls with that where possible but don't have anything to show yet.

Apart from that, the reason I like the filter solution is because it only minimally changes the logic within GlotPress, so if it were for me, I'd still go with that solution.

@toolstack
Contributor

I think we'd have to leave both in for the time being for backwards compatibility. There may be plugins that expect it so they can filter the existing permissions.

It might work to remove them by adding a default filter for the translation permission check on gp_pre_user_can and simply returning the result of a permission check on translation-set.

@akirk
Contributor
akirk commented Sep 13, 2016

It might work to remove them by adding a default filter for the translation permission check on gp_pre_user_can and simply returning the result of a permission check on translation-set.

Yeah, that's what I have been doing but I am currently struggling with the cascade of filters when trying to implement the cross-locale plugin.

@toolstack
Contributor

It might be easier just to remove the default filter and replace it in the plugin.

@ocean90 ocean90 modified the milestone: 2.3, 2.2 Sep 13, 2016
@akirk
Contributor
akirk commented Sep 16, 2016

@toolstack thanks for the suggestion, that made it easier indeed. See the new implementation in #538.

@akirk
Contributor
akirk commented Sep 16, 2016 edited

In this new implementation I've taken up both of your suggestions and made it work by leaving $can_approve in place and adding a $can_approve_translation = $this->can( 'approve', 'translation', $translation->id, [ 'translation' => $translation ] ); where necessary (i.e. everywhere the 'translation-row' template is used).

@akirk akirk added a commit to akirk/GlotPress that referenced this issue Nov 18, 2016
@akirk akirk Fixes #537 by adding the filters that allow the implementation of Cro…
…ss-PTEs
7d54947
@akirk akirk added a commit to akirk/GlotPress that referenced this issue Nov 18, 2016
@akirk akirk Fixes #537 by adding the filters that allow the implementation of Cro…
…ss-PTEs
cd29192
@ocean90 ocean90 closed this in b74848e Nov 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment