Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type Mithril from 'mithril';
import Badge from 'flarum/common/components/Badge';
import highlight from 'flarum/common/helpers/highlight';
import type AtMentionFormat from './formats/AtMentionFormat';
import sortGroups from 'flarum/common/utils/sortGroups';

export default class GroupMention extends MentionableModel<Group, AtMentionFormat> {
type(): string {
Expand All @@ -13,9 +14,11 @@ export default class GroupMention extends MentionableModel<Group, AtMentionForma

initialResults(): Group[] {
return Array.from(
app.store.all<Group>('groups').filter((g: Group) => {
return g.id() !== Group.GUEST_ID && g.id() !== Group.MEMBER_ID;
})
sortGroups(
app.store.all<Group>('groups').filter((g: Group) => {
return g.id() !== Group.GUEST_ID && g.id() !== Group.MEMBER_ID;
})
)
);
}

Expand Down
2 changes: 2 additions & 0 deletions framework/core/js/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
"mithril": "^2.2",
"nanoid": "^3.1.30",
"punycode": "^2.1.1",
"sortablejs": "^1.14.0",
"textarea-caret": "^3.1.0",
"throttle-debounce": "^3.0.1"
},
Expand All @@ -27,6 +28,7 @@
"@types/jquery": "^3.5.10",
"@types/mithril": "^2.0.8",
"@types/punycode": "^2.1.0",
"@types/sortablejs": "^1.15.9",
"@types/textarea-caret": "^3.0.1",
"@types/ua-parser-js": "^0.7.36",
"bundlewatch": "^0.3.2",
Expand Down
95 changes: 95 additions & 0 deletions framework/core/js/src/admin/components/GroupBar.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import sortable from 'sortablejs';

import app from '../../admin/app';
import Component, { ComponentAttrs } from '../../common/Component';
import GroupBadge from '../../common/components/GroupBadge';
import Icon from '../../common/components/Icon';
import EditGroupModal from './EditGroupModal';
import sortGroups from '../../common/utils/sortGroups';

import type Group from '../../common/models/Group';
import type Mithril from 'mithril';

export interface IGroupBarAttrs extends ComponentAttrs {
groups: Group[];
}

/**
* A reusable component for displaying a list of groups in the admin interface,
* with the ability to edit and reorder them.
*
* @property {Group[]} groups Required. Groups to display.
*
* @example
* ```ts
* const availableGroups = app.store
* .all<Group>('groups')
* .filter((group) => [Group.GUEST_ID, Group.MEMBER_ID].indexOf(group.id()!) === -1);
*
* <GroupBar groups={availableGroups} />
* ```
*/
export default class GroupBar<CustomAttrs extends IGroupBarAttrs = IGroupBarAttrs> extends Component<CustomAttrs> {
groups: Group[] = [];

oninit(vnode: Mithril.Vnode<CustomAttrs, this>) {
super.oninit(vnode);

this.groups = sortGroups(this.attrs.groups);
}

view(): JSX.Element {
return (
<div className="GroupBar" oncreate={this.onGroupBarCreate.bind(this)}>
{this.groups.map((group) => (
<button className="Button Group" type="button" data-id={group.id()} onclick={() => app.modal.show(EditGroupModal, { group })}>
<GroupBadge group={group} className="Group-icon" label={null} />
<span className="Group-name">{group.namePlural()}</span>
</button>
))}
<button className="Button Group Group--add" type="button" onclick={() => app.modal.show(EditGroupModal)}>
<Icon name="fas fa-plus" className="Group-icon" />
<span className="Group-name">{app.translator.trans('core.admin.permissions.new_group_button')}</span>
</button>
</div>
);
}

onGroupBarCreate(vnode: Mithril.VnodeDOM) {
sortable.create(vnode.dom as HTMLElement, {
group: 'groups',
delay: 50,
delayOnTouchOnly: true,
touchStartThreshold: 5,
animation: 150,
swapThreshold: 0.65,
dragClass: 'Group-Sortable-Dragging',
ghostClass: 'Group-Sortable-Placeholder',

filter: '.Group--add',
onMove: (evt) => !evt.related.classList.contains('Group--add'),

onSort: () => this.onSortUpdate(),
});
}

onSortUpdate() {
const order = this.$('.Group:not(.Group--add)')
.map(function () {
return $(this).data('id');
})
.get();

order.forEach((id, i) => {
app.store.getById<Group>('groups', id)?.pushData({
attributes: { position: i },
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


app.request({
url: app.forum.attribute('apiUrl') + '/groups/order',
method: 'POST',
body: { order },
});
}
}
30 changes: 15 additions & 15 deletions framework/core/js/src/admin/components/PermissionDropdown.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import Separator from '../../common/components/Separator';
import Group from '../../common/models/Group';
import Badge from '../../common/components/Badge';
import GroupBadge from '../../common/components/GroupBadge';
import sortGroups from '../../common/utils/sortGroups';
import Mithril from 'mithril';

function badgeForId(id: string) {
Expand Down Expand Up @@ -95,21 +96,20 @@ export default class PermissionDropdown<CustomAttrs extends IPermissionDropdownA
// These groups are defined above, appearing first in the list.
const excludedGroups = [Group.ADMINISTRATOR_ID, Group.GUEST_ID, Group.MEMBER_ID];

const groupButtons = app.store
.all<Group>('groups')
.filter((group) => !excludedGroups.includes(group.id()!))
.map((group) => (
<Button
icon={groupIds.includes(group.id()!) ? 'fas fa-check' : true}
onclick={(e: MouseEvent) => {
if (e.shiftKey) e.stopPropagation();
this.toggle(group.id()!);
}}
disabled={this.isGroupDisabled(group.id()!) && this.isGroupDisabled(Group.MEMBER_ID) && this.isGroupDisabled(Group.GUEST_ID)}
>
{badgeForId(group.id()!)} {group.namePlural()}
</Button>
));
const availableGroups = sortGroups(app.store.all<Group>('groups').filter((group) => !excludedGroups.includes(group.id()!)));

const groupButtons = availableGroups.map((group) => (
<Button
icon={groupIds.includes(group.id()!) ? 'fas fa-check' : true}
onclick={(e: MouseEvent) => {
if (e.shiftKey) e.stopPropagation();
this.toggle(group.id()!);
}}
disabled={this.isGroupDisabled(group.id()!) && this.isGroupDisabled(Group.MEMBER_ID) && this.isGroupDisabled(Group.GUEST_ID)}
>
{badgeForId(group.id()!)} {group.namePlural()}
</Button>
));

children.push(...groupButtons);
}
Expand Down
20 changes: 4 additions & 16 deletions framework/core/js/src/admin/components/PermissionsPage.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
import app from '../../admin/app';
import GroupBadge from '../../common/components/GroupBadge';
import EditGroupModal from './EditGroupModal';
import GroupBar from './GroupBar';
import Group from '../../common/models/Group';
import PermissionGrid from './PermissionGrid';
import AdminPage from './AdminPage';
import Icon from '../../common/components/Icon';
import SettingDropdown from './SettingDropdown';

export default class PermissionsPage extends AdminPage {
Expand All @@ -18,22 +16,12 @@ export default class PermissionsPage extends AdminPage {
}

content() {
const availableGroups = app.store.all<Group>('groups').filter((group) => [Group.GUEST_ID, Group.MEMBER_ID].indexOf(group.id()!) === -1);

return (
<>
<div className="PermissionsPage-groups">
{app.store
.all<Group>('groups')
.filter((group) => [Group.GUEST_ID, Group.MEMBER_ID].indexOf(group.id()!) === -1)
.map((group) => (
<button className="Button Group" type="button" onclick={() => app.modal.show(EditGroupModal, { group })}>
<GroupBadge group={group} className="Group-icon" label={null} />
<span className="Group-name">{group.namePlural()}</span>
</button>
))}
<button className="Button Group Group--add" type="button" onclick={() => app.modal.show(EditGroupModal)}>
<Icon name="fas fa-plus" className="Group-icon" />
<span className="Group-name">{app.translator.trans('core.admin.permissions.new_group_button')}</span>
</button>
<GroupBar groups={availableGroups} />
</div>

<div className="PermissionsPage-permissions">
Expand Down
1 change: 1 addition & 0 deletions framework/core/js/src/common/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import './utils/abbreviateNumber';
import './utils/escapeRegExp';
import './utils/string';
import './utils/throttleDebounce';
import './utils/sortGroups';
import './utils/Stream';
import './utils/SubtreeRetainer';
import './utils/setRouteWithForcedRefresh';
Expand Down
40 changes: 17 additions & 23 deletions framework/core/js/src/common/components/EditUserModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import FormModal, { IFormModalAttrs } from '../../common/components/FormModal';
import Button from './Button';
import GroupBadge from './GroupBadge';
import Group from '../models/Group';
import sortGroups from '../utils/sortGroups';
import extractText from '../utils/extractText';
import ItemList from '../utils/ItemList';
import Stream from '../utils/Stream';
Expand All @@ -22,6 +23,7 @@ export default class EditUserModal<CustomAttrs extends IEditUserModalAttrs = IEd
protected setPassword!: Stream<boolean>;
protected password!: Stream<string>;
protected groups: Record<string, Stream<boolean>> = {};
protected availableGroups: Group[] = [];

oninit(vnode: Mithril.Vnode<CustomAttrs, this>) {
super.oninit(vnode);
Expand All @@ -36,10 +38,11 @@ export default class EditUserModal<CustomAttrs extends IEditUserModalAttrs = IEd

const userGroups = user.groups() || [];

app.store
.all<Group>('groups')
.filter((group) => ![Group.GUEST_ID, Group.MEMBER_ID].includes(group.id()!))
.forEach((group) => (this.groups[group.id()!] = Stream(userGroups.includes(group))));
this.availableGroups = sortGroups(app.store.all<Group>('groups').filter((group) => ![Group.GUEST_ID, Group.MEMBER_ID].includes(group.id()!)));

this.availableGroups.forEach((group) => {
this.groups[group.id()!] = Stream(userGroups.includes(group));
});
}

className() {
Expand Down Expand Up @@ -139,25 +142,16 @@ export default class EditUserModal<CustomAttrs extends IEditUserModalAttrs = IEd
<div className="Form-group EditUserModal-groups">
<label>{app.translator.trans('core.lib.edit_user.groups_heading')}</label>
<div>
{Object.keys(this.groups)
.map((id) => app.store.getById<Group>('groups', id))
.filter(Boolean)
.map(
(group) =>
// Necessary because filter(Boolean) doesn't narrow out falsy values.
group && (
<label className="checkbox">
<input
type="checkbox"
bidi={this.groups[group.id()!]}
disabled={
group.id() === Group.ADMINISTRATOR_ID && (this.attrs.user === app.session.user || !this.userIsAdmin(app.session.user))
}
/>
<GroupBadge group={group} label={null} /> {group.nameSingular()}
</label>
)
)}
{this.availableGroups.map((group) => (
<label className="checkbox">
<input
type="checkbox"
bidi={this.groups[group.id()!]}
disabled={group.id() === Group.ADMINISTRATOR_ID && (this.attrs.user === app.session.user || !this.userIsAdmin(app.session.user))}
/>
<GroupBadge group={group} label={null} /> {group.nameSingular()}
</label>
))}
</div>
</div>,
10
Expand Down
4 changes: 4 additions & 0 deletions framework/core/js/src/common/models/Group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,8 @@ export default class Group extends Model {
isHidden() {
return Model.attribute<boolean>('isHidden').call(this);
}

position() {
return Model.attribute<number>('position').call(this);
}
}
14 changes: 14 additions & 0 deletions framework/core/js/src/common/utils/sortGroups.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import type Group from '../models/Group';

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;
});
}
5 changes: 5 additions & 0 deletions framework/core/js/webpack.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,9 @@ module.exports = merge(config(), {
output: {
library: 'flarum.core',
},
resolve: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@DavideIadeluca DavideIadeluca Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

alias: {
sortablejs: require.resolve('sortablejs/Sortable.js'),
},
},
});
13 changes: 9 additions & 4 deletions framework/core/less/admin/PermissionsPage.less
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
.PermissionsPage-groups {
.GroupBar {
background: var(--control-bg);
border-radius: var(--border-radius);
display: block;
overflow-x: auto;
display: flex;
gap: 10px;
flex-wrap: wrap;
padding: 10px;
border-radius: var(--border-radius);
}
.Group-Sortable-Placeholder {
outline: 2px dashed var(--body-bg);
border-radius: var(--border-radius);
}
.Group {
width: 90px;
display: inline-block;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

/*
* This file is part of Flarum.
*
* For detailed copyright and license information, please view the
* LICENSE file that was distributed with this source code.
*/

use Illuminate\Database\Schema\Blueprint;
use Illuminate\Database\Schema\Builder;

return [
'up' => function (Builder $schema) {
$schema->table('groups', function (Blueprint $table) {
$table->integer('position')->after('is_hidden')->nullable();
});

$db = $schema->getConnection();

$ids = $db->table('groups')
->orderBy('id')
->pluck('id');

$position = 0;
foreach ($ids as $id) {
$db->table('groups')
->where('id', $id)
->update(['position' => $position]);

$position++;
}
},

'down' => function (Builder $schema) {
$schema->table('groups', function (Blueprint $table) {
$table->dropColumn('position');
});
}
];
Loading
Loading