[2.x] feat(core, mentions): implement ability to order groups#4350
[2.x] feat(core, mentions): implement ability to order groups#4350DavideIadeluca wants to merge 12 commits intoflarum:2.xfrom
Conversation
DavideIadeluca
changed the title
imorland
left a comment
imorland
left a comment
There was a problem hiding this comment.
Thanks for this PR — group ordering is a welcome addition and the overall structure is good. A few issues need addressing before this can merge, from correctness problems down to minor polish. See inline comments for specifics.
Summary of required changes:
sortGroups: handlenullpositions (same pattern assortTags)GroupBar: removeabstractor use a concrete subclass inPermissionsPage— cannot instantiate an abstract class directlyGroupResource: add->readonly()to thepositionfieldOrderGroupsController: add input validation (type-check, verify IDs are real groups, protect Guest/Member)- Webpack alias: this fix belongs in
flarum-webpack-config, not just core — otherwise extension authors who bundle sortablejs will hit the same Terser error - Clean up the two
chore: debuggingcommits before merge (squash or rebase) - Error handling on the
app.requestinonSortUpdate— revert on failure
Questions for you:
- Is
GroupBarintended to be a base class that extensions subclass (as the twofactor example suggests), or should it be directly usable? If the former,PermissionsPageneeds a concreteDefaultGroupBar extends GroupBar. - For the webpack fix — do you want to explore moving sortablejs to a Flarum external (exposed from core bundle) rather than an alias? That would be cleaner long-term.
| import type Group from '../models/Group'; | ||
|
|
||
| export default function sortGroups(groups: Group[]) { | ||
| return groups.slice().sort((a, b) => a.position() - b.position()); |
There was a problem hiding this comment.
This will behave incorrectly when position() returns null — null - null coerces to 0 in JS, and null - 5 coerces to -5, so groups with a null position sort as if they have position 0, jumping to the front. This happens for new groups created after an ordering, and for any group not included in the order payload.
Please handle null positions explicitly, following the same pattern as sortTags.tsx:
export default function sortGroups(groups: Group[]) {
return groups.slice().sort((a, b) => {
const aPos = a.position();
const bPos = b.position();
if (aPos === null && bPos === null) return 0;
if (aPos === null) return 1;
if (bPos === null) return -1;
return aPos - bPos;
});
}| } | ||
|
|
||
| export default abstract class GroupBar<CustomAttrs extends IGroupBarAttrs = IGroupBarAttrs> extends Component<CustomAttrs> { | ||
| groups: Group[] = []; |
There was a problem hiding this comment.
GroupBar is declared abstract but PermissionsPage uses it directly via <GroupBar groups={availableGroups} />. TypeScript will compile this, but Mithril instantiates components at runtime — instantiating an abstract class throws in JS.
Either:
- Remove
abstractif this is meant to be directly usable as-is, or - Keep it abstract and introduce a concrete subclass (
class DefaultGroupBar extends GroupBar {}) forPermissionsPageto use — which would also give extensions a clean extension point without having to extend an abstract class directly.
| ghostClass: 'Group-Sortable-Placeholder', | ||
|
|
||
| filter: '.Group--add', | ||
| onMove: (evt) => !evt.related.classList.contains('Group--add'), |
There was a problem hiding this comment.
onSort receives an event e that is declared but never used. Either use it or remove the parameter:
onSort: () => this.onSortUpdate(),| app.store.getById<Group>('groups', id)?.pushData({ | ||
| attributes: { position: i }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
app.request is fire-and-forget with no error handling. If the request fails, SortableJS has already reordered the DOM and the store has been updated via pushData, leaving the UI and server in an inconsistent state.
At minimum, revert the store positions on failure:
const previousPositions = order.map((id, i) => ({
id,
position: app.store.getById<Group>("groups", id)?.position() ?? null,
}));
app.request({ ... }).catch(() => {
previousPositions.forEach(({ id, position }) => {
app.store.getById<Group>("groups", id)?.pushData({ attributes: { position } });
});
m.redraw();
});| output: { | ||
| library: 'flarum.core', | ||
| }, | ||
| resolve: { |
There was a problem hiding this comment.
This alias fixes the Terser/ESM issue for core, but any extension that bundles sortablejs directly will hit the same error. The fix should live in flarum-webpack-config so it applies everywhere, or sortablejs should be exposed as an external from the core bundle (similar to how mithril/jQuery are handled), so extensions can reference it without bundling their own copy.
Could you look into moving this to flarum-webpack-config? Or would you prefer we handle that separately?
| ->writable(), | ||
| Schema\Boolean::make('isHidden') | ||
| ->writable(), | ||
| Schema\Integer::make('position') |
There was a problem hiding this comment.
position should be ->readonly(). Without it, the field is writable via PATCH /api/groups/{id}, bypassing the ordering endpoint and its admin check entirely. Position should only be settable through POST /api/groups/order.
Schema\Integer::make(position)
->nullable()
->readonly(),| if ($order === null) { | ||
| return new EmptyResponse(422); | ||
| } | ||
|
|
There was a problem hiding this comment.
A few input validation gaps here:
- No type check —
$ordercould be a non-array (e.g. a string), which would makeforeachthrow. - No validation that the IDs in
$orderare real group IDs — an invalid ID silently does nothing but the reset at line 33 (update position = nullfor all) still runs. - Guest (ID 2) and Member (ID 3) groups could be included in the payload, which would set positions on groups the frontend intentionally excludes. Consider ignoring or rejecting those IDs.
Suggested additions:
if (\!is_array($order)) {
return new EmptyResponse(422);
}| * @property string|null $color | ||
| * @property string|null $icon | ||
| * @property bool $is_hidden | ||
| * @property int $position |
There was a problem hiding this comment.
The migration adds ->nullable() and GroupFactory defaults to null, so this should be @property int|null $position.
| .Group-Sortable-Placeholder { | ||
| outline: 2px dashed var(--body-bg); | ||
| border-radius: var(--border-radius); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation — the rest of the file uses 2-space indent but .Group-Sortable-Placeholder uses 4-space. Please normalise.
2 participants
Fixes #0000
Changes proposed in this pull request:
flarum/tagsGroupBarcomponent which contains thesortablejsintegration. Extensions like https://github.com/imorland/flarum-ext-twofactor/blob/2.x/js/src/admin/components/ExtractedGroupBar.tsx could consume this new componentReviewers should focus on:
This was the error message (reproducible both in CI and when running
yarn buildlocally):Screenshot
https://github.com/user-attachments/assets/8dcb047b-bd52-435e-a5b1-a5da46673664
Necessity
Confirmed
composer test).Required changes: