@ggppdk @csthomas as promised. Please test/check.

@andrepereiradasilva

you found all places,
that calculate the "$canOrder" in the record listing template files: (tmpl/default.php)

Just the removal of the variable "$canOrder" is not best,
instead we should fix the wrong check, since

• $canOrder is ACL check or core.edit.state on something (currently on wrong thing you found this)
• $saveOrder is the check if current sorting is "*.ordering"

We should change, as you previously had pointed out, to something that makes sense, the component asset

$canOrder  = $user->authorise('core.edit.state', 'com_content.article');

$canOrder  = $user->authorise('core.edit.state', 'com_content');

If you do not do this

then the ordering will appear "inactive" for users with no core.edit.state on the component
(it will appear inactive because it does core.edit.state on the component too),

but the user despite drag-icons being inactive, can still drag the field,
ordering is not saved but a call is made towards server with no effect),
and this is confusing to the user and wrong

Adding to my previous reply, if category filter is acctive:
Instead of

$canOrder  = $user->authorise('core.edit.state', 'com_content');

use the category asset: (case we have soft-deny on component and then allow for specific category)

$filter_catid = $this->state->get('filter.category_id');
$canOrder = !$filter_catid ?
    $user->authorise('core.edit.state', 'com_content') :
    $user->authorise('core.edit.state', 'com_content.category.'.$filter_catid);

Maybe this will require some small changes for save order task too, to check category asset ?

you will never have a perfect behaviour in hathor, only if you introduce core.edit.order which imho would be an overkill just for this.

The thing is, in hathor, in the order column header is checking core.edit.state of the component but the rows are checking (and correctly) the category or item core.edit.state.

So, in the current behaviour, it can lead to a place where the user as no core.edit.state in the component (the order save button will not appear), but does have core.edit.state in some items (the order field will not appear).

We can, of course, mantain the current behavour, that is also not perfect.

I will do that them, ie, remove the not used $canOrder from isis and just correct the acl asset name in hathor. This will make it work like it was, but remove unused code and correct wrong acl asset names.

ok done, should be fine now

It looks good now,
but i am a little confused, why have you updated only the hathor template overrides ?

and what do you think about instead of:

$canOrder  = $user->authorise('core.edit.state', 'com_content');

using the category asset: (case we have soft-deny on component and then allow for specific category)

$filter_catid = $this->state->get('filter.category_id');
$canOrder = !$filter_catid ?
    $user->authorise('core.edit.state', 'com_content') :
    $user->authorise('core.edit.state', 'com_content.category.'.$filter_catid);

Thus have even more appropriate ACL checks that will work with soft-deny at component and core.edit.state granted on individual categories

I checked just now and it seems that

• there is no need to update any FOF files,
• and no need to update anything on saveOrderAjax task either of legacy admin controler, because it calls the saveorder() of the legacy admin model that already checks individual record (e.g. article) assets thus no need to update anything there too

but i am a little confused, why have you updated only the hathor template overrides ?

because $canOrderis only used in hathor templates, what i remvoed in isis in code that doesn't do anything because isis does not have the save butoon in the ordering column title.

and what do you think about instead of:

Honestly hathor is going to be removed in 4.0, i don't see the need to change it's behaviout at this point.

because $canOrderis only used in hathor templates, what i remvoed in isis in code that doesn't do anything because isis does not have the save butoon in the ordering column title.

no, it should be used for adding or not adding the JS of drag and drop re-ordering
if you do not use it, there are 2 problems

• the drag elements appear disabled due to the "inactive" class being added, but user can still drag them
• and when they is a drag operation, a server call is made (which of course fails to set the order because the saveorder() of legacy admin model checks ACL and disallows it)

@ggppdk open any of those isis files and do a Ctrl+F for canOrder and you will see the variable is not being used.

Yes, exactly, i have seen,

i suggested that we do add usage of it, to prevent the adding of the drag and drop JS

So do it as a separate pull request. We have really got to stop making "super" pull requests fixing a bunch of stuff because one pull request exposed something else.

ehhehe agree ... i have been "victim" of some pull requests that exposed a lot of problems

@mbabker
you are right,
just it seemed relevant / appropriate to add the better checks that i suggested, that also consider the category filter, in this one, but i see your point lets add them in a different PR,

This PR is good in what it tries to do, remove meaningless wrong assets checks, and i will test / review it

I ll make a PR of adding the checks i suggested above, during week, if someone else does not do
Example for com_content, that should work correctly with no changes:

$filter_catid = $this->state->get('filter.category_id');
$canOrder = !$filter_catid ?
    $user->authorise('core.edit.state', 'com_content') :
    $user->authorise('core.edit.state', 'com_content.category.'.$filter_catid);

$saveOrder = $listOrder == 'a.ordering';

if ($canOrder && $saveOrder)
{
    $saveOrderingUrl = 'index.php?option=com_content&task=articles.saveOrderAjax&tmpl=component';
    JHtml::_('sortablelist.sortable', 'articleList', 'adminForm', strtolower($listDirn), $saveOrderingUrl);
}

I have tested this item ✅ successfully on be82ddf

@andrepereiradasilva thanks! Tested this and can't find different behaviour after applying the patch.

I have tested this item ✅ successfully on be82ddf

RTC