plg_groups_members: order member roles and save canned deny replies

[denphi]

Two changes to the group accept/decline screen, both for managers.

Member roles listed in the sidebar had no order at all: the query carried no ORDER BY, so they came back in creation order. #__xgroups_roles gains an ordering column, and a manager can drag roles into place, saved over AJAX. A "Sort automatically" button applies the order these lists are usually kept in - plain names alphabetically, then names holding a year in date order, reading months, seasons ("Fall 2025") and prefixes ("Class of 2026"). New roles are appended rather than jumping ahead of an arranged list, and the sidebar, each member's role list, the assign-role form and the messages plugin's role picker all read the same order. Existing roles start at 0 and so list by name until a manager sorts them.

Denying membership offered one empty textarea, retyped for every applicant. A group's managers can now save replies they send often and pick one from the deny form, editing it before it goes out. They live in the group's params, so no new table, and the email path is unchanged.

[nkissebe]

Severity: medium — Group::update() returns false when its UPDATE fails, but the return value is dropped here and the manager is redirected with PLG_GROUPS_MEMBERS_DENY_RESPONSES_SAVED ("Canned responses saved.") regardless.

This matters more than usual for this feature because #__xgroups.params is a text column (65,535 bytes) and the responses are unbounded free text — views/responses/tmpl/default.php puts no maxlength on the textarea and savedenyresponses() caps neither the length of a response nor how many there are. Concrete scenario: a manager pastes a few long boilerplate rejection letters. On a strict-mode server the UPDATE is rejected, update() returns false, and the manager is told the responses were saved when nothing was written. On a non-strict server MySQL silently truncates the column mid-JSON, so the whole params blob becomes unparsable and the group loses every other setting stored there (join policy, plugin access, discoverability, membership terms) — again while reporting success.

Suggest checking the return value and refusing an oversized payload up front rather than letting the column truncate, e.g. serialize first, bail with an error if strlen() > 65535, and re-render the form (handing back what was typed) when update() returns false.

[nkissebe]

Severity: low — $response['success'] is set to true unconditionally once auth and the token pass, even when the loop did no useful work.

roles[] is never validated: (int) $id turns junk into 0, and 0 can never match a row, so the UPDATEs quietly affect nothing. jQuery UI's serialize() makes this reachable rather than theoretical — when it can't match any element ids against its expression it emits the single pair roles[]= (see the if (!str.length && o.key) branch in core/assets/js/jquery.ui.js). PHP then receives [''], every id casts to 0, no row is touched, and the handler answers {"success":true}. The JS in members.js takes that as confirmation and leaves the dragged order on screen, so the manager believes the new order was saved; it reverts on the next page load.

Filtering the ids and treating an empty list as a failure makes the silent case visible:

$ids = array_values(array_filter(array_map('intval', Request::getArray('roles', array(), 'post'))));

if (empty($ids))
{
	$response['message'] = Lang::txt('PLG_GROUPS_MEMBERS_ROLE_ORDER_ERROR');
}
else
{
	// ... existing loop over $ids, then $response['success'] = true;
}

[nkissebe]

Severity: low — the matching column is missing from core/bootstrap/Install/sql/mysql/schema.sql, which still declares #__xgroups_roles as (id, gidNumber, name, permissions) (around line 5954).

That file is the canonical fresh-install schema, and the convention in this tree is to change both: the sibling feature in the same area (45f83df, PR #1915) shipped Migration20260810000000ComGroups.php and a schema.sql update in the same commit. Leaving it out means the file no longer describes a current database, so anything that builds or diffs a schema straight from it (test fixtures, a restored baseline, the next regeneration) is missing ordering, and every query added by this PR — getRoles(), the per-member query in views/browse/tmpl/default.php, and plg_groups_messages — is a fatal Unknown column 'ordering' until the migration runs against it.

Adding `ordering` int(11) NOT NULL DEFAULT 0 after `name` in schema.sql keeps the two in step.

[nkissebe]

Severity: low — this breaks the "roles list by name until a manager sorts them" promise the migration docblock makes, the first time a role is added.

After the migration every role sits at ordering = 0, so getRoles()'s ORDER BY ordering ASC, name ASC lists a never-reordered group alphabetically. The moment a manager adds one more role, the max here is 0, the new role gets 1, and it lands below every existing role while the rest stay alphabetical — a group that never touched ordering now has one role stuck at the bottom, with no UI hint why. Concrete: a group with roles "Alumni", "Faculty", "Staff" (all at 0) adds "Advisors"; the sidebar shows Alumni, Faculty, Staff, Advisors.

The method comment says a new role should not jump ahead of "roles a manager has already put in order" — in a group where nothing has been ordered there is no arranged list to protect. Only appending when the group actually has an order satisfies both intents:

$last = self::blank()
	->whereEquals('gidNumber', (int) $this->get('gidNumber'))
	->order('ordering', 'desc')
	->limit(1)
	->row();

// A group whose roles have never been ordered keeps listing by name
$this->set('ordering', ((int) $last->get('ordering') > 0)
	? (int) $last->get('ordering') + 1
	: 0);

Flagging rather than asserting — if new-roles-always-last is the deliberate choice, then the migration docblock is the thing that needs rewording.

[nkissebe]

Severity: low — this link navigates away from the deny form in the same tab, silently discarding whatever the manager has already typed into #reason.

The form carries the selected users through (users[] into denyresponses(), back out via the redirect in savedenyresponses()), but the reply textarea below is not carried anywhere. Concrete scenario: a manager writes a paragraph of custom explanation, realises it is worth keeping for next time, clicks "Manage canned responses", pastes it in and saves — the redirect lands them back on a fresh deny form and the text they typed in the original textarea is gone. It is recoverable only because they happened to have pasted it into the response form; if they click the link before pasting, it is lost outright.

Same applies to the "Add some" link on line 57 in the empty case, though there the textarea is more likely to still be empty. Cheapest fix is target="_blank" on these two links so the deny form stays put; stashing #reason in sessionStorage before navigating and restoring it on load would preserve the single-tab flow.