[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 21 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.
| 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();
});There was a problem hiding this comment.
app.request is fire-and-forget with no error handling.
This is not quite correct. app.request() actually has a ton of error handling. But yes it's true that, should the request fail, that the UI and Server are in an inconsistent state.
This is the same behaviour as for when ordering Tags or other implementations of app.request() like dismissing flags or changing permissions. Should those implementations also be adapted to make sure that the UI state is correctly reflected when the server returns an error? If yes let's create a follow up issue for this, If no I'd say the implementation in this PR just follows existing standards
| 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?
There was a problem hiding this comment.
Yeah I'd prefer if we handle this separately. Open to moving this PR back to draft until that is done or proceed with this PR and make the webpack fix later. Can create a follow up issue if you woud like
| 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);
}There was a problem hiding this comment.
No type check — $order could be a non-array (e.g. a string), which would make foreach throw.
Changed and added a test in 91de357
No validation that the IDs in $order are real group IDs — an invalid ID silently does nothing but the reset at line 33 (update position = null for all) still runs.
This is related to the quirk I described in the PR description. The DB Migration does set a position for all groups, including Member and Guest: so this behaviour is kind of desired with the above quirk. Essentially in the DB migration doesn't know for a fact that ID's 2 and 3 are indeed the Guest and Member Group without some runtime check which validates the class constants of Group.
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.
Idk if this is a bug or a feature. The implementation in PermissionsPage does ignore those Groups, but the new component does technically support them as groups are passed into the component. There might be some cases where third party customizations want control over this. Not sure if it's worth it to restrict this
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: