[ActionsLog] Improve UX for loggable extensions checkboxes with a select all/none SEE #22699

[cyrezdev]

EDIT

new PR here: #22699

Pull Request for Issue #22627.

Summary of Changes

• Remove default attribute for loggable_extensions (Fix [com_actionlogs] After deselecting all events to disable logging, checkboxes remain ticked #22627 UX issue)
• Add a "Select All/None" for loggable extensions to help selecting all or clearing all on one click.

Testing Instructions

• See issue [com_actionlogs] After deselecting all events to disable logging, checkboxes remain ticked #22627
• Test actions logging options, all selected, some selected, none selected.

[Quy]

Remove and start/change line 62 $html =

[cyrezdev]

Thanks @Quy !

I did the changes.

I thought when use concatenation operators, the first one should be empty (to help to keep code diffs minimal)...
Was it changed as a rule or i was wrong ?

Note: some other core files still use first empty (PR needed for code standards ?).
eg https://github.com/joomla/joomla-cms/blob/staging/libraries/vendor/joomla/uri/src/AbstractUri.php#L150

		$uri = '';
		$uri .= $parts & static::SCHEME ? (!empty($this->scheme) ? $this->scheme . '://' : '') : '';
		$uri .= $parts & static::USER ? $this->user : '';
		$uri .= $parts & static::PASS ? (!empty($this->pass) ? ':' : '') . $this->pass . (!empty($this->user) ? '@' : '') : '';
		$uri .= $parts & static::HOST ? $this->host : '';
		$uri .= $parts & static::PORT ? (!empty($this->port) ? ':' : '') . $this->port : '';
		$uri .= $parts & static::PATH ? $this->path : '';
		$uri .= $parts & static::QUERY ? (!empty($query) ? '?' . $query : '') : '';
		$uri .= $parts & static::FRAGMENT ? (!empty($this->fragment) ? '#' . $this->fragment : '') : '';

[Quy]

Add space before .=

[mbabker]

Looks fine on review, requires tests

[Quy]

It is still an issue when editing a profile.

Edit a Super User profile
Go to User Actions Log Options
Select None
Click Save

All checkboxes are checked.

[cyrezdev]

@Quy Oh! I didn't even see there was this field in super user edit...

To be honnest, i don't see yet how the core extensions are set by default, as no default attribute in plugin system actionlogs form...

[cyrezdev]

@Quy Did you test super user with latest staging including #21983 ?

[Quy]

Yes tested with latest staging including #21983.

[SharkyKZ]

The plugin will need to be adjusted. But the field also needs to remove checked attribute from its options.

Test case:
Save com_actionlogs configuration with some or all events selected.
Deselect all events and save again.
No events are saved but all events appear checked.

[cyrezdev]

Thanks @SharkyKZ
Do you have tested this PR ?

Test case:
Save com_actionlogs configuration with some or all events selected.
Deselect all events and save again.
No events are saved but all events appear checked.

With this PR, it works for me with com_actionlogs configuration as i've removed there the default attribute from config.xml.
But in plugin system actionlogs, no default attribute for user edit form actionlogs.xml
So it should act as for global options as same form field used.

The issue is with this PR not really the same as it was before.

Maybe in menu/plugin, the issue is the save function does not allow empty submitted data, and then replace by previously saved data.

@SharkyKZ your help could be appreciated to know if a change in form data processed to be done in the plugin, or if another issue elsewhere...

[cyrezdev]

@SharkyKZ I've found why this PR does not have effect on super user edit!

In plugin, it is forced to save only if isset. But if empty, it won't update extensions column, and so keep previous setting.

Fix
Adding the following else statement for this if statement here: https://github.com/joomla/joomla-cms/pull/21983/files#diff-bbf838ec8885d4ea43274f7b94e17808R343:

else
{
	$values[] = $this->db->quoteName('extensions') . ' = ""';
}

Would fix issue.

Note: i can add this change to this PR if you think it's ok with this ?

[SharkyKZ]

#21983 was written to maintain the existing behavior of form fields. See #21983 (comment).

Your fix seems OK. But maybe we should store empty JSON array ([]) instead of an empty string. IDK about this.

Also the field itself needs updating to remove checked attribute by default. Otherwise, after deselecting all options and saving they appear checked again, even when default value is not set. See this part

joomla-cms/administrator/components/com_actionlogs/models/fields/logtype.php

Line 125 in a4479d3

$tmp = array('checked' => true);

[mbabker]

Your fix seems OK. But maybe we should store empty JSON array ([]) instead of an empty string. IDK about this.

If a column or data source is expecting a JSON string, then it should always be stored as something that doesn't cause json_decode() to set an error flag. We've got too many places in core where this is being hacked around already.

[cyrezdev]

Will redo this PR with changes needed (i did something wrong with udpating this one, and better to get things clear) ;-)

[cyrezdev]

PR redo here: #22699
Thanks for comments and help here! 👍