Change 644388 had a related patch set uploaded (by MarkAHershberger; owner: MarkAHershberger):
[mediawiki/extensions/ApprovedRevs@master] Add ability to disable bits of {{#approvable_by}} if needed.

https://gerrit.wikimedia.org/r/644388

@MarkAHershberger - could you explain this in more detail? I don't really understand the requested feature, or the patch.

We want users to be able to say

{{#approvable_by: group = approvers}}

or

{{#approvable_by: group = editors}}

but not

{{#approvable_by: user = sam}}

With this change, we can restrict the approvable_by parser function to groups by putting

$egApprovedRevsApprovableBy['users'] = false;

in our LocalSettings.php

Okay, now I get it. It would be easier to just disable the "users" parameter when handling #approvable_by... I suppose the downside there is that you can't do anything about #approvable_by calls that already exist and already include a "users" param, but that doesn't seem like a big deal. Or is it?

Also, it seems like 80% of this patch is just refactoring the code... if you want to refactor the code, could you create a separate patch for that?

In T269060#6661223, @Yaron_Koren wrote:

Okay, now I get it. It would be easier to just disable the "users" parameter when handling #approvable_by... I suppose the downside there is that you can't do anything about #approvable_by calls that already exist and already include a "users" param, but that doesn't seem like a big deal. Or is it?

The code uses the message approvedrevs-not-allowed when a disallowed parameter is used. Which means that if you have a wiki with #approvable_by calls already in it, they'll be marked when the page is re-rendered.

If the users param is disabled then the page property approvedrevs-approver-users is ignored so any restrictions it introduced are also ignored.

Also, it seems like 80% of this patch is just refactoring the code... if you want to refactor the code, could you create a separate patch for that?

This will work if I can refactor the code and then introduce the change. Would that be acceptable?

That's fair about refactoring - so let's talk about this change first. I understand that this patch will work better for wikis that already have #approvable_by with the "users" param. The question is, is that superior handling worth the extra complexity? It doesn't seem that way - after all, it's easy to just get rid of any "users" params that already exist on the wiki.

Just getting rid of the parameters on the wiki does not make them unavailable to users. We want users to use the group parameter, but we do not want to let them use the user parameter since that will change the behavior in an undesired way.

We do not want users to feel like they can own a page, but we do want to allow groups of people to be able to agree and review content. And we want different groups to be able to review different content.

I think you may have misunderstood what I'm saying. Here's the basic idea: disallowing the "users" param seems reasonable, but that can be done by just modifying the code that handles #approvable_by - which seems quite a bit simpler than how the current patch does it. Wouldn't you agree?

The code could be simpler, but then it wouldn't address the page properties that already exist. Without that, the possibility of ghost user restrictions remains.

My patch addresses all the points where user restrictions have been made and disables all of them, if desired. This is safer in the long run.

Well, right - "ghost restrictions" (or really, "ghost permissions") is what I was talking about when I said that it's easy for an admin to go through, find "users" params, and delete them. Don't you think that's a good enough solution? Especially since I don't think an unauthorized approver can do much damage.

No, I don't think that is a good enough solution. That's why I was thorough in cleaning out the permissions.

Why would we expect an admin to clean up the wiki when we can just tell the wiki to ignore the functionality we do not want and highlight the attempts to use that functionality?

This is all part of working smarter, not working harder.

Well, because I think it unnecessarily complicates the code, for the sake of backward compatibility that I think is minor.