Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
80 changes: 80 additions & 0 deletions framework/core/js/src/admin/components/GroupBar.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
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[];
}

export default abstract class GroupBar<CustomAttrs extends IGroupBarAttrs = IGroupBarAttrs> extends Component<CustomAttrs> {
groups: Group[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. Remove abstract if this is meant to be directly usable as-is, or
  2. Keep it abstract and introduce a concrete subclass (class DefaultGroupBar extends GroupBar {}) for PermissionsPage to use — which would also give extensions a clean extension point without having to extend an abstract class directly.


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'),
Copy link
Member

Choose a reason for hiding this comment

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

onSort receives an event e that is declared but never used. Either use it or remove the parameter:

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


onSort: (e) => 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();
});


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);
}
}
5 changes: 5 additions & 0 deletions framework/core/js/src/common/utils/sortGroups.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import type Group from '../models/Group';

export default function sortGroups(groups: Group[]) {
return groups.slice().sort((a, b) => a.position() - b.position());
Copy link
Member

Choose a reason for hiding this comment

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

This will behave incorrectly when position() returns nullnull - 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;
  });
}

}
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?

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);
}
Copy link
Member

Choose a reason for hiding this comment

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

Inconsistent indentation — the rest of the file uses 2-space indent but .Group-Sortable-Placeholder uses 4-space. Please normalise.

.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