From 97bf164592696599c142f3e58e127bfa3e1e2580 Mon Sep 17 00:00:00 2001 From: Renato Atilio Date: Tue, 24 Feb 2026 10:08:34 -0300 Subject: [PATCH 0001/1051] FEATURE: consolidate composer toolbar list and add check list (#37336) This PR merges the bulleted and numbered list buttons into a single Lists dropdown menu in the composer toolbar and introduces a new checklist option to the dropdown. * Combines individual list buttons into a single "Lists" dropdown * Adds a new checklist option within the list menu (integrated via the checklist plugin) * ProseMirror/Rich Editor: * Added logic for checklist continuation on Enter and exit on double Enter * Implemented Backspace handling to manage checkbox removal and line joining * Added cursor positioning constraints to prevent selection before the checkbox * Added handler to toggle the check when clicked Updated `addComposerToolbarPopupMenuOption` to allow plugins to register custom options specifically within a popup menu through its identifier image --- .../common/toolbar-popup-menu-options.scss | 18 +- config/locales/client.en.yml | 1 + .../discourse/app/components/d-editor.gjs | 4 +- .../components/toolbar-popup-menu-options.gjs | 2 +- .../lib/composer/rich-editor-extensions.js | 1 + .../discourse/app/lib/composer/toolbar.js | 99 ++-- frontend/discourse/app/lib/plugin-api.gjs | 14 + frontend/discourse/app/services/composer.js | 24 +- .../components/prosemirror-editor.gjs | 2 + .../prosemirror/lib/text-manipulation.js | 60 +- .../integration/components/d-editor-test.gjs | 25 +- .../discourse/initializers/checklist.js | 16 + .../javascripts/lib/rich-editor-extension.js | 532 +++++++++++++++++- .../assets/stylesheets/checklist.scss | 3 +- .../checklist/config/locales/client.en.yml | 5 + .../page_objects/components/rich_checklist.rb | 40 ++ .../spec/system/rich_editor_extension_spec.rb | 141 +++++ .../integration/rich-editor-extension-test.js | 19 +- .../composer/prosemirror_toolbar_spec.rb | 10 +- 19 files changed, 913 insertions(+), 103 deletions(-) create mode 100644 plugins/checklist/config/locales/client.en.yml create mode 100644 plugins/checklist/spec/system/page_objects/components/rich_checklist.rb create mode 100644 plugins/checklist/spec/system/rich_editor_extension_spec.rb diff --git a/app/assets/stylesheets/common/toolbar-popup-menu-options.scss b/app/assets/stylesheets/common/toolbar-popup-menu-options.scss index 0ea31c18b2e23..e0f1cba36efb1 100644 --- a/app/assets/stylesheets/common/toolbar-popup-menu-options.scss +++ b/app/assets/stylesheets/common/toolbar-popup-menu-options.scss @@ -30,7 +30,9 @@ } } -.toolbar-menu__options-content { +.toolbar-menu__options-content, +.toolbar-menu__list-content, +.toolbar-menu__heading-content { .dropdown-menu { .btn { &.--active { @@ -58,16 +60,10 @@ } } -.toolbar-menu__heading-content { +.toolbar-menu__heading-content, +.toolbar-menu__list-content { .dropdown-menu { .btn { - &.--active { - .d-button-label__active-icon { - visibility: visible; - margin-left: auto; - } - } - &:hover, &:focus, &:focus-visible { @@ -106,7 +102,11 @@ font-size: var(--font-down-1-rem); } } + } +} +.toolbar-menu__heading-content { + .dropdown-menu { .btn[data-name="heading-1"] { .d-button-label__text, .d-icon[class*="d-icon-discourse"] { diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 94e83205b5304..968dccf86fcc5 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3153,6 +3153,7 @@ en: upload_description: "enter upload description here" olist_title: "Numbered list" ulist_title: "Bulleted list" + list_title: "Lists" list_item: "List item" toggle_direction: "Toggle direction" apply_wrap_title: "Apply wrap" diff --git a/frontend/discourse/app/components/d-editor.gjs b/frontend/discourse/app/components/d-editor.gjs index cc59f83a96ba8..2d5caba62010c 100644 --- a/frontend/discourse/app/components/d-editor.gjs +++ b/frontend/discourse/app/components/d-editor.gjs @@ -196,13 +196,13 @@ export default class DEditor extends Component { keymap[sc] = () => { const customAction = shortcuts[sc].shortcutAction; + const toolbarEvent = this.newToolbarEvent(); if (customAction) { - const toolbarEvent = this.newToolbarEvent(); if (!button.condition || button.condition(toolbarEvent)) { customAction(toolbarEvent); } } else { - button.action(button); + button.action(toolbarEvent); } return false; }; diff --git a/frontend/discourse/app/components/toolbar-popup-menu-options.gjs b/frontend/discourse/app/components/toolbar-popup-menu-options.gjs index 2256c3901da71..94679072d6b09 100644 --- a/frontend/discourse/app/components/toolbar-popup-menu-options.gjs +++ b/frontend/discourse/app/components/toolbar-popup-menu-options.gjs @@ -33,7 +33,7 @@ export default class ToolbarPopupmenuOptions extends Component { } #convertMenuOption(content) { - if (content.condition) { + if (content.condition !== false) { const label = this.#calculateLabel(content); const title = this.#calculateTitle(content); diff --git a/frontend/discourse/app/lib/composer/rich-editor-extensions.js b/frontend/discourse/app/lib/composer/rich-editor-extensions.js index 04150df20c9cd..dd471a9254653 100644 --- a/frontend/discourse/app/lib/composer/rich-editor-extensions.js +++ b/frontend/discourse/app/lib/composer/rich-editor-extensions.js @@ -32,6 +32,7 @@ * @property {typeof import('prosemirror-history')} pmHistory * @property {typeof import('prosemirror-transform')} pmTransform * @property {typeof import('prosemirror-commands')} pmCommands + * @property {typeof import('prosemirror-schema-list')} pmSchemaList * @property {import('prosemirror-model').Schema} schema * @property {() => PluginContext} getContext */ diff --git a/frontend/discourse/app/lib/composer/toolbar.js b/frontend/discourse/app/lib/composer/toolbar.js index 872ff25cf9419..3853ffb96fd3e 100644 --- a/frontend/discourse/app/lib/composer/toolbar.js +++ b/frontend/discourse/app/lib/composer/toolbar.js @@ -1,5 +1,5 @@ // @ts-check -import { action } from "@ember/object"; +import { customPopupMenuOptions } from "discourse/lib/composer/custom-popup-menu-options"; import { translateModKey } from "discourse/lib/utilities"; import { waitForClosedKeyboard } from "discourse/lib/wait-for-keyboard"; import { PLATFORM_KEY_MODIFIER } from "discourse/services/keyboard-shortcuts"; @@ -134,6 +134,10 @@ export class ToolbarBase { // Popup menu option item shortcut bindings and title text. if (buttonAttrs.popupMenu) { + // Default action passes toolbarEvent to option.action + buttonAttrs.popupMenu.action ??= (option) => + option.action(this.context.newToolbarEvent()); + buttonAttrs.popupMenu.options()?.forEach((option) => { if (option.shortcut) { const shortcutKeyTranslated = translateModKey( @@ -181,6 +185,8 @@ export class ToolbarBase { * Standard editor toolbar with default buttons */ export default class Toolbar extends ToolbarBase { + #listOptions; + constructor(opts) { super(opts); @@ -269,7 +275,8 @@ export default class Toolbar extends ToolbarBase { return false; }, - action: this.onHeadingMenuAction.bind(this), + action: (toolbarEvent) => + toolbarEvent.applyHeading(headingLevel, "heading"), }); } headingOptions.push({ @@ -281,11 +288,10 @@ export default class Toolbar extends ToolbarBase { showActiveIcon: true, shortcut: "Alt+0", active: ({ state }) => state?.inParagraph, - action: this.onHeadingMenuAction.bind(this), + action: (toolbarEvent) => toolbarEvent.applyHeading(0, "heading"), }); return headingOptions; }, - action: this.onHeadingMenuAction.bind(this), }, }); @@ -325,28 +331,25 @@ export default class Toolbar extends ToolbarBase { active: ({ state }) => state.inCode || state.inCodeBlock, }); - this.addButton({ - id: "bullet", - group: "extras", - icon: "list-ul", - shortcut: "Shift+8", - title: "composer.ulist_title", - perform: (e) => e.applyList("* ", "list_item"), - active: ({ state }) => state.inBulletList, - }); - this.addButton({ id: "list", group: "extras", - icon: "list-ol", - shortcut: "Shift+7", - title: "composer.olist_title", - perform: (e) => - e.applyList( - (i) => (!i ? "1. " : `${parseInt(i, 10) + 1}. `), - "list_item" - ), - active: ({ state }) => state.inOrderedList, + active: ({ state }) => { + return this.getListPopupMenuOptions().some((option) => + option.active({ state }) + ); + }, + icon: ({ state }) => { + return ( + this.getListPopupMenuOptions().find((option) => + option.active({ state }) + )?.icon || "list-ul" + ); + }, + title: "composer.list_title", + popupMenu: { + options: () => this.getListPopupMenuOptions(), + }, }); } @@ -362,16 +365,46 @@ export default class Toolbar extends ToolbarBase { } } - @action - onHeadingMenuAction(menuItem) { - let level; - - if (menuItem.name === "heading-paragraph") { - level = 0; - } else { - level = parseInt(menuItem.name.split("-")[1], 10); - } + getListPopupMenuOptions() { + this.#listOptions ??= [ + { + name: "list-bullet", + icon: "list-ul", + label: "composer.ulist_title", + shortcut: "Shift+8", + showActiveIcon: true, + active: ({ state }) => state?.inBulletList, + action: (toolbarEvent) => { + if ( + !toolbarEvent.commands?.toggleBulletList || + !toolbarEvent.commands.toggleBulletList() + ) { + toolbarEvent.applyList("* ", "list_item"); + } + }, + }, + { + name: "list-ordered", + icon: "list-ol", + label: "composer.olist_title", + shortcut: "Shift+7", + showActiveIcon: true, + active: ({ state }) => state?.inOrderedList, + action: (toolbarEvent) => { + if ( + !toolbarEvent.commands?.toggleOrderedList || + !toolbarEvent.commands.toggleOrderedList() + ) { + toolbarEvent.applyList( + (i) => (!i ? "1. " : `${parseInt(i, 10) + 1}. `), + "list_item" + ); + } + }, + }, + ...customPopupMenuOptions.filter((option) => option.menu === "list"), + ]; - this.context.newToolbarEvent().applyHeading(level, "heading"); + return this.#listOptions; } } diff --git a/frontend/discourse/app/lib/plugin-api.gjs b/frontend/discourse/app/lib/plugin-api.gjs index 188055f9ae03b..9e4506cc78ab9 100644 --- a/frontend/discourse/app/lib/plugin-api.gjs +++ b/frontend/discourse/app/lib/plugin-api.gjs @@ -848,6 +848,8 @@ class _PluginApi { * @returns {boolean} - Whether the button should be displayed. * * @param {Object} opts - An Object. + * @param {string} [opts.menu] - Target menu: 'list' for list dropdown, omit for options popup (default). + * @param {string} [opts.name] - Unique identifier for the option. * @param {string} opts.icon - The name of the FontAwesome icon to display for the button. * @param {string} opts.label - The I18n translation key for the button's label. * @param {string} opts.shortcut - The keyboard shortcut to apply, NOTE: this will unconditionally add CTRL/META key (eg: m means CTRL+m). @@ -866,6 +868,18 @@ class _PluginApi { * return composer.editingPost; * } * }); + * + * @example + * // Add option to list dropdown + * api.addComposerToolbarPopupMenuOption({ + * menu: 'list', + * name: 'my-custom-list', + * icon: 'list-check', + * label: 'my_plugin.custom_list', + * action: (toolbarEvent) => { + * toolbarEvent.applyList("- [x] ", "list_item"); + * } + * }); **/ addComposerToolbarPopupMenuOption(opts) { addPopupMenuOption(opts); diff --git a/frontend/discourse/app/services/composer.js b/frontend/discourse/app/services/composer.js index bd563c676c24e..65823de552155 100644 --- a/frontend/discourse/app/services/composer.js +++ b/frontend/discourse/app/services/composer.js @@ -487,13 +487,14 @@ export default class ComposerService extends Service { }), ]; - return options - .concat( - customPopupMenuOptions - .map((option) => this._setupPopupMenuOption({ ...option })) - .filter((o) => o) - ) - .concat(secondaryOptions); + return [ + ...options, + ...customPopupMenuOptions + .filter((option) => !option.menu) + .map((option) => this._setupPopupMenuOption({ ...option })) + .filter(Boolean), + ...secondaryOptions, + ]; } } @@ -725,12 +726,9 @@ export default class ComposerService extends Service { menuItem ); if (typeof menuItem.action === "function") { - // note: due to the way args are passed to actions we need - // to create the explicity toolbarEvent as a fallback for no - // event - // Long term we want to avoid needing this awkwardness and pass - // the event explicitly - return menuItem.action(this.toolbarEvent || toolbarEvent); + // toolbarEvent is passed when triggered via keyboard shortcut, + // otherwise fall back to stored toolbarEvent from menu open + return menuItem.action(toolbarEvent ?? this.toolbarEvent); } else { return ( this.actions?.[menuItem.action]?.bind(this) || // Legacy-style contributions from themes/plugins diff --git a/frontend/discourse/app/static/prosemirror/components/prosemirror-editor.gjs b/frontend/discourse/app/static/prosemirror/components/prosemirror-editor.gjs index c938cb0d77fa6..a5424d09785bc 100644 --- a/frontend/discourse/app/static/prosemirror/components/prosemirror-editor.gjs +++ b/frontend/discourse/app/static/prosemirror/components/prosemirror-editor.gjs @@ -17,6 +17,7 @@ import * as ProsemirrorHistory from "prosemirror-history"; import { history } from "prosemirror-history"; import { keymap } from "prosemirror-keymap"; import * as ProsemirrorModel from "prosemirror-model"; +import * as ProsemirrorSchemaList from "prosemirror-schema-list"; import * as ProsemirrorState from "prosemirror-state"; import { EditorState } from "prosemirror-state"; import * as ProsemirrorTransform from "prosemirror-transform"; @@ -106,6 +107,7 @@ export default class ProsemirrorEditor extends Component { pmView: ProsemirrorView, pmHistory: ProsemirrorHistory, pmTransform: ProsemirrorTransform, + pmSchemaList: ProsemirrorSchemaList, pmCommands: ProsemirrorCommands, getContext: () => ({ placeholder: this.args.placeholder, diff --git a/frontend/discourse/app/static/prosemirror/lib/text-manipulation.js b/frontend/discourse/app/static/prosemirror/lib/text-manipulation.js index 4b94070275c26..afed1b07813ee 100644 --- a/frontend/discourse/app/static/prosemirror/lib/text-manipulation.js +++ b/frontend/discourse/app/static/prosemirror/lib/text-manipulation.js @@ -5,7 +5,11 @@ import { isEmpty } from "@ember/utils"; import { TrackedObject } from "@ember-compat/tracked-built-ins"; import { lift, setBlockType, toggleMark, wrapIn } from "prosemirror-commands"; import { Slice } from "prosemirror-model"; -import { liftListItem, sinkListItem } from "prosemirror-schema-list"; +import { + liftListItem, + sinkListItem, + wrapInList, +} from "prosemirror-schema-list"; import { Selection, TextSelection } from "prosemirror-state"; import { bind } from "discourse/lib/decorators"; import escapeRegExp from "discourse/lib/escape-regexp"; @@ -186,26 +190,63 @@ export default class ProsemirrorTextManipulation { applyList(_selection, head, exampleKey) { let command; - const isInside = (type) => { + const findParentList = () => { const $from = this.view.state.selection.$from; for (let depth = $from.depth; depth > 0; depth--) { - const parent = $from.node(depth); - if (parent.type === type) { - return true; + const node = $from.node(depth); + if ( + node.type === this.schema.nodes.bullet_list || + node.type === this.schema.nodes.ordered_list + ) { + return { + node, + pos: $from.before(depth), + type: node.type, + }; } } - return false; + return null; }; if (exampleKey === "list_item") { - const nodeType = + const targetType = head === "* " ? this.schema.nodes.bullet_list : this.schema.nodes.ordered_list; - command = isInside(this.schema.nodes.list_item) ? lift : wrapIn(nodeType); + const parentList = findParentList(); + + if (parentList) { + if (parentList.type === targetType) { + // Same list type - toggle off (lift) + command = liftListItem(this.schema.nodes.list_item); + } else { + // Different list type - convert selection + // We achieve this by lifting out of the current list, then wrapping in the new type + command = (state, dispatch) => { + if (dispatch) { + const liftCmd = liftListItem(this.schema.nodes.list_item); + let lifted = false; + liftCmd(state, (tr) => { + dispatch(tr); + lifted = true; + }); + + if (lifted) { + // If lift succeeded, now wrap in the new list type + // We need to re-fetch state from view since dispatch updated it + wrapInList(targetType)(this.view.state, dispatch); + } + } + return true; + }; + } + } else { + // Not in a list - wrap in the target type + command = wrapInList(targetType); + } } else if (exampleKey === "blockquote_text") { - command = isInside(this.schema.nodes.blockquote) + command = inNode(this.view.state, this.schema.nodes.blockquote) ? lift : wrapIn(this.schema.nodes.blockquote); } else { @@ -213,6 +254,7 @@ export default class ProsemirrorTextManipulation { } command?.(this.view.state, this.view.dispatch); + this.focus(); } applyHeading(_selection, level) { diff --git a/frontend/discourse/tests/integration/components/d-editor-test.gjs b/frontend/discourse/tests/integration/components/d-editor-test.gjs index 03f8cabe239c6..f5280e7927630 100644 --- a/frontend/discourse/tests/integration/components/d-editor-test.gjs +++ b/frontend/discourse/tests/integration/components/d-editor-test.gjs @@ -567,12 +567,14 @@ third line` async function (assert, textarea) { const example = i18n("composer.list_item"); - await click(`button.bullet`); + await click(`button.list`); + await click('.btn[data-name="list-bullet"]'); assert.strictEqual(this.value, `hello world.\n\n* ${example}`); assert.strictEqual(textarea.selectionStart, 14); assert.strictEqual(textarea.selectionEnd, 16 + example.length); - await click(`button.bullet`); + await click(`button.list`); + await click('.btn[data-name="list-bullet"]'); assert.strictEqual(this.value, `hello world.\n\n${example}`); } ); @@ -583,12 +585,14 @@ third line` textarea.selectionStart = 6; textarea.selectionEnd = 11; - await click(`button.bullet`); + await click(`button.list`); + await click('.btn[data-name="list-bullet"]'); assert.strictEqual(this.value, `hello\n\n* world\n\n.`); assert.strictEqual(textarea.selectionStart, 7); assert.strictEqual(textarea.selectionEnd, 14); - await click(`button.bullet`); + await click(`button.list`); + await click('.btn[data-name="list-bullet"]'); assert.strictEqual(this.value, `hello\n\nworld\n\n.`); assert.strictEqual(textarea.selectionStart, 7); assert.strictEqual(textarea.selectionEnd, 12); @@ -603,12 +607,14 @@ third line` textarea.selectionStart = 0; textarea.selectionEnd = 20; - await click(`button.bullet`); + await click(`button.list`); + await click('.btn[data-name="list-bullet"]'); assert.strictEqual(this.value, "Hello\n\nWorld\n\nEvil"); assert.strictEqual(textarea.selectionStart, 0); assert.strictEqual(textarea.selectionEnd, 18); - await click(`button.bullet`); + await click(`button.list`); + await click('.btn[data-name="list-bullet"]'); assert.strictEqual(this.value, "* Hello\n\n* World\n\n* Evil"); assert.strictEqual(textarea.selectionStart, 0); assert.strictEqual(textarea.selectionEnd, 24); @@ -621,11 +627,13 @@ third line` const example = i18n("composer.list_item"); await click(`button.list`); + await click('.btn[data-name="list-ordered"]'); assert.strictEqual(this.value, `hello world.\n\n1. ${example}`); assert.strictEqual(textarea.selectionStart, 14); assert.strictEqual(textarea.selectionEnd, 17 + example.length); await click(`button.list`); + await click('.btn[data-name="list-ordered"]'); assert.strictEqual(this.value, `hello world.\n\n${example}`); assert.strictEqual(textarea.selectionStart, 14); assert.strictEqual(textarea.selectionEnd, 14 + example.length); @@ -639,11 +647,13 @@ third line` textarea.selectionEnd = 11; await click(`button.list`); + await click('.btn[data-name="list-ordered"]'); assert.strictEqual(this.value, `hello\n\n1. world\n\n.`); assert.strictEqual(textarea.selectionStart, 7); assert.strictEqual(textarea.selectionEnd, 15); await click(`button.list`); + await click('.btn[data-name="list-ordered"]'); assert.strictEqual(this.value, `hello\n\nworld\n\n.`); assert.strictEqual(textarea.selectionStart, 7); assert.strictEqual(textarea.selectionEnd, 12); @@ -659,11 +669,13 @@ third line` textarea.selectionEnd = 18; await click(`button.list`); + await click('.btn[data-name="list-ordered"]'); assert.strictEqual(this.value, "1. Hello\n\n2. World\n\n3. Evil"); assert.strictEqual(textarea.selectionStart, 0); assert.strictEqual(textarea.selectionEnd, 27); await click(`button.list`); + await click('.btn[data-name="list-ordered"]'); assert.strictEqual(this.value, "Hello\n\nWorld\n\nEvil"); assert.strictEqual(textarea.selectionStart, 0); assert.strictEqual(textarea.selectionEnd, 18); @@ -678,6 +690,7 @@ third line` textarea.selectionEnd = 8; await click("button.list"); + await click('.btn[data-name="list-ordered"]'); assert.strictEqual(this.value, "1. existing"); document.execCommand("undo"); diff --git a/plugins/checklist/assets/javascripts/discourse/initializers/checklist.js b/plugins/checklist/assets/javascripts/discourse/initializers/checklist.js index d65ee040d1e30..a187c8b183f9e 100644 --- a/plugins/checklist/assets/javascripts/discourse/initializers/checklist.js +++ b/plugins/checklist/assets/javascripts/discourse/initializers/checklist.js @@ -10,6 +10,22 @@ function initializePlugin(api) { if (siteSettings.checklist_enabled) { api.decorateCookedElement(checklistSyntax); api.registerRichEditorExtension(richEditorExtension); + + api.addComposerToolbarPopupMenuOption({ + menu: "list", + name: "list-checklist", + icon: "list-check", + label: "checklist.composer.checklist", + showActiveIcon: true, + active: ({ state }) => state?.inCheckList, + action: (toolbarEvent) => { + if (toolbarEvent.commands?.toggleChecklist) { + toolbarEvent.commands.toggleChecklist(); + } else { + toolbarEvent.applyList("- [ ] ", "list_item"); + } + }, + }); } } diff --git a/plugins/checklist/assets/javascripts/lib/rich-editor-extension.js b/plugins/checklist/assets/javascripts/lib/rich-editor-extension.js index adb8a2320078f..f9366eedf5d20 100644 --- a/plugins/checklist/assets/javascripts/lib/rich-editor-extension.js +++ b/plugins/checklist/assets/javascripts/lib/rich-editor-extension.js @@ -1,8 +1,5 @@ /** @type {RichEditorExtension} */ const extension = { - // TODO(renato): make the checkbox clickable - // TODO(renato): auto-continue checkbox list on ENTER - // TODO(renato): apply .has-checkbox style to the
  • to avoid :has nodeSpec: { check: { attrs: { checked: { default: false } }, @@ -23,9 +20,7 @@ const extension = { parseDOM: [ { tag: "span.chcklst-box", - getAttrs: (dom) => { - return { checked: hasCheckedClass(dom.className) }; - }, + getAttrs: (dom) => ({ checked: hasCheckedClass(dom.className) }), }, ], }, @@ -34,12 +29,16 @@ const extension = { inputRules: [ { match: /(^|\s)\[(x? ?)]$/, - handler: (state, match, start, end) => - state.tr.replaceWith( - start + match[1].length, - end, - state.schema.nodes.check.create({ checked: match[2] === "x" }) - ), + handler: (state, match, start, end) => { + const checkNode = state.schema.nodes.check.create({ + checked: match[2] === "x", + }); + const spaceNode = state.schema.text(" "); + return state.tr.replaceWith(start + match[1].length, end, [ + checkNode, + spaceNode, + ]); + }, }, ], @@ -58,6 +57,515 @@ const extension = { state.write(node.attrs.checked ? "[x]" : "[ ]"); }, }, + + plugins({ + pmState: { Plugin }, + pmView: { Decoration, DecorationSet }, + schema, + utils: { changedDescendants }, + }) { + const checkType = schema.nodes.check; + const listItemType = schema.nodes.list_item; + const bulletListType = schema.nodes.bullet_list; + + const startsWithCheck = (node) => + node?.isTextblock && node.firstChild?.type === checkType; + + const findBulletListContext = ($from) => { + for (let depth = $from.depth; depth > 0; depth--) { + if ($from.node(depth).type === listItemType) { + if ($from.node(depth - 1)?.type === bulletListType) { + return { bulletListDepth: depth - 1, listItemDepth: depth }; + } + return null; + } + } + return null; + }; + + const inBulletListItem = (doc, pos) => + findBulletListContext(doc.resolve(pos)) !== null; + + const ensureSpaceAfterChecks = (tr, oldState, newState) => { + const positionsToInsert = []; + + changedDescendants(oldState.doc, newState.doc, (node, pos) => { + if (!startsWithCheck(node) || !inBulletListItem(newState.doc, pos)) { + return; + } + + const secondChild = node.childCount > 1 ? node.child(1) : null; + const hasSpaceAfter = + secondChild?.isText && secondChild.text?.[0] === " "; + + if (!hasSpaceAfter) { + positionsToInsert.push(pos + 1 + node.firstChild.nodeSize); + } + }); + + for (let i = positionsToInsert.length - 1; i >= 0; i--) { + tr.insert(positionsToInsert[i], schema.text(" ")); + } + }; + + const exitChecklist = (tr, ctx) => { + const { bulletListDepth, listItemDepth } = ctx; + const { selection } = tr; + const { $from } = selection; + const bulletList = $from.node(bulletListDepth); + const listItemIndex = $from.index(bulletListDepth); + const prevListItem = bulletList.child(listItemIndex - 1); + const bulletListStart = $from.before(bulletListDepth); + + tr.delete($from.before(listItemDepth), $from.after(listItemDepth)); + + let prevOffset = 1; + for (let i = 0; i < listItemIndex - 1; i++) { + prevOffset += bulletList.child(i).nodeSize; + } + const prevItemStart = tr.mapping.map(bulletListStart + prevOffset); + tr.delete(prevItemStart, prevItemStart + prevListItem.nodeSize); + + const mappedListStart = tr.mapping.map(bulletListStart); + const listAfter = tr.doc.nodeAt(mappedListStart); + + if (listAfter && listAfter.childCount > 0) { + const listEnd = mappedListStart + listAfter.nodeSize; + tr.insert(listEnd, schema.nodes.paragraph.create()); + tr.setSelection( + selection.constructor.near(tr.doc.resolve(listEnd + 1)) + ); + } else { + tr.replaceWith( + mappedListStart, + mappedListStart + (listAfter?.nodeSize || 0), + schema.nodes.paragraph.create() + ); + tr.setSelection( + selection.constructor.near(tr.doc.resolve(mappedListStart + 1)) + ); + } + return tr; + }; + + const handleChecklistContinuation = (tr, transactions) => { + if (!transactions.some((t) => t.docChanged)) { + return null; + } + + const { selection } = tr; + const { $from } = selection; + + if (!selection.empty) { + return null; + } + + const parent = $from.parent; + if (!parent.isTextblock || parent.content.size !== 0) { + return null; + } + + const ctx = findBulletListContext($from); + if (!ctx) { + return null; + } + + const bulletList = $from.node(ctx.bulletListDepth); + const listItemIndex = $from.index(ctx.bulletListDepth); + if (listItemIndex === 0) { + return null; + } + + const prevParagraph = bulletList.child(listItemIndex - 1).firstChild; + if (!startsWithCheck(prevParagraph)) { + return null; + } + + if (prevParagraph.content.size > 2) { + const checkNode = checkType.create({ checked: false }); + tr.insert($from.pos, [checkNode, schema.text(" ")]); + tr.setSelection( + selection.constructor.near(tr.doc.resolve($from.pos + 2)) + ); + return tr; + } + + return exitChecklist(tr, ctx); + }; + + const adjustCursorPosition = (tr) => { + const { doc, selection } = tr; + const { $from } = selection; + + if (!selection.empty) { + return null; + } + + const parent = $from.parent; + if (!startsWithCheck(parent) || !inBulletListItem(doc, $from.pos)) { + return null; + } + + const checkSize = parent.firstChild.nodeSize; + const secondChild = parent.childCount > 1 ? parent.child(1) : null; + const hasSpaceAfter = + secondChild?.isText && secondChild.text?.[0] === " "; + const minPos = hasSpaceAfter ? checkSize + 1 : checkSize; + + if ($from.parentOffset < minPos) { + tr.setSelection( + selection.constructor.near(doc.resolve($from.start() + minPos)) + ); + return tr; + } + + return null; + }; + + return [ + new Plugin({ + props: { + handleClickOn(view, pos, node, nodePos) { + if (node.type.name === "check") { + view.dispatch( + view.state.tr.setNodeMarkup(nodePos, null, { + checked: !node.attrs.checked, + }) + ); + return true; + } + return false; + }, + + handleKeyDown(view, event) { + if (event.key !== "Backspace" && event.key !== "ArrowLeft") { + return false; + } + + const { state, dispatch } = view; + const { selection } = state; + const { $from } = selection; + + // Only handle at position 2 (right after check+space) in a checklist + if ( + !selection.empty || + $from.parentOffset !== 2 || + !startsWithCheck($from.parent) || + !inBulletListItem(state.doc, $from.pos) + ) { + return false; + } + + if (event.key === "Backspace") { + const ctx = findBulletListContext($from); + + const checkStart = $from.start(); + let tr = state.tr.delete(checkStart, checkStart + 2); + + if (ctx) { + const listItemPos = tr.mapping.map( + $from.before(ctx.listItemDepth) + ); + const $listItem = tr.doc.resolve(listItemPos); + + if ($listItem.nodeBefore?.type === listItemType) { + tr = tr.join(listItemPos, 2); + } + } + + dispatch(tr); + return true; + } + + const beforeTextblock = $from.before(); + if (beforeTextblock > 0) { + dispatch( + state.tr.setSelection( + selection.constructor.near( + state.doc.resolve(beforeTextblock), + -1 + ) + ) + ); + return true; + } + + return false; + }, + }, + + appendTransaction(transactions, oldState, newState) { + const isFullReplace = transactions.some( + (t) => + t.steps.length === 1 && + t.steps[0].from === 0 && + t.steps[0].to === oldState.doc.content.size + ); + if (isFullReplace) { + return null; + } + + const tr = newState.tr; + ensureSpaceAfterChecks(tr, oldState, newState); + + return ( + handleChecklistContinuation(tr, transactions) ?? + adjustCursorPosition(tr) ?? + (tr.docChanged ? tr : null) + ); + }, + }), + + // Decoration plugin to add has-checkbox class to checklist items + new Plugin({ + props: { + decorations(state) { + const decorations = []; + + state.doc.descendants((node, pos, parent) => { + if ( + node.type === listItemType && + parent?.type === bulletListType && + startsWithCheck(node.firstChild) + ) { + decorations.push( + Decoration.node(pos, pos + node.nodeSize, { + class: "has-checkbox", + }) + ); + } + }); + + return DecorationSet.create(state.doc, decorations); + }, + }, + }), + ]; + }, + + commands: ({ schema, pmSchemaList }) => { + const checkType = schema.nodes.check; + const bulletListType = schema.nodes.bullet_list; + const orderedListType = schema.nodes.ordered_list; + const listItemType = schema.nodes.list_item; + + const listItemHasCheck = (listItem) => { + const p = listItem.firstChild; + return p?.isTextblock && p.firstChild?.type === checkType; + }; + + const findListContext = (state) => { + const { $from, $to } = state.selection; + for (let depth = $from.depth; depth > 0; depth--) { + if ($from.node(depth).type === listItemType) { + const list = $from.node(depth - 1); + if (list?.type === bulletListType || list?.type === orderedListType) { + return { + listDepth: depth - 1, + listItemDepth: depth, + listType: list.type, + list, + listStart: $from.before(depth - 1), + $from, + $to, + }; + } + } + } + return null; + }; + + const forEachSelectedItem = (ctx, callback) => { + const { list, listStart, $from, $to } = ctx; + const collapsed = $from.pos === $to.pos; + + list.forEach((item, offset) => { + const itemStart = listStart + 1 + offset; + const itemEnd = itemStart + item.nodeSize; + const inSelection = collapsed + ? $from.pos >= itemStart && $from.pos <= itemEnd + : !(itemEnd <= $from.pos || itemStart >= $to.pos); + + if (inSelection) { + callback(item, itemStart); + } + }); + }; + + const hasCheckInSelection = (ctx) => { + if (!ctx || ctx.listType !== bulletListType) { + return false; + } + let found = false; + forEachSelectedItem(ctx, (item) => { + if (listItemHasCheck(item)) { + found = true; + } + }); + return found; + }; + + const removeChecksFromSelection = (state, ctx) => { + const toDelete = []; + forEachSelectedItem(ctx, (item, itemStart) => { + if (listItemHasCheck(item)) { + const textblock = item.firstChild; + const checkSize = textblock.firstChild.nodeSize; + const second = textblock.childCount > 1 ? textblock.child(1) : null; + const hasSpace = second?.isText && second.text?.[0] === " "; + toDelete.push({ + from: itemStart + 2, + to: itemStart + 2 + checkSize + (hasSpace ? 1 : 0), + }); + } + }); + + let tr = state.tr; + for (let i = toDelete.length - 1; i >= 0; i--) { + tr = tr.delete(toDelete[i].from, toDelete[i].to); + } + return tr; + }; + + const addChecksToSelection = (state, ctx) => { + const toInsert = []; + forEachSelectedItem(ctx, (item, itemStart) => { + if (!listItemHasCheck(item) && item.firstChild?.isTextblock) { + toInsert.push(itemStart + 2); + } + }); + + let tr = state.tr; + let offset = 0; + for (const pos of toInsert) { + const check = checkType.create({ checked: false }); + const space = schema.text(" "); + tr = tr.insert(pos + offset, [check, space]); + offset += check.nodeSize + space.nodeSize; + } + return tr; + }; + + return { + toggleBulletList() { + return (state, dispatch) => { + const ctx = findListContext(state); + if (hasCheckInSelection(ctx)) { + if (dispatch) { + dispatch(removeChecksFromSelection(state, ctx)); + } + return true; + } + return false; + }; + }, + + toggleOrderedList() { + return (state, dispatch, view) => { + const ctx = findListContext(state); + if (!hasCheckInSelection(ctx)) { + return false; + } + + if (!dispatch) { + return true; + } + + const liftListItem = pmSchemaList?.liftListItem; + const wrapInList = pmSchemaList?.wrapInList; + if (!liftListItem || !wrapInList) { + return false; + } + + dispatch(removeChecksFromSelection(state, ctx)); + if (view) { + liftListItem(listItemType)(view.state, dispatch); + wrapInList(orderedListType)(view.state, dispatch); + } + return true; + }; + }, + + toggleChecklist() { + return (state, dispatch, view) => { + const ctx = findListContext(state); + const wrapInList = pmSchemaList?.wrapInList; + const liftListItem = pmSchemaList?.liftListItem; + + if (hasCheckInSelection(ctx)) { + if (!dispatch || !liftListItem) { + return !!liftListItem; + } + dispatch(removeChecksFromSelection(state, ctx)); + if (view) { + liftListItem(listItemType)(view.state, dispatch); + } + return true; + } + + if (ctx?.listType === bulletListType) { + if (dispatch) { + dispatch(addChecksToSelection(state, ctx)); + } + return true; + } + + if (ctx?.listType === orderedListType) { + if (!dispatch || !liftListItem || !wrapInList) { + return !!(liftListItem && wrapInList); + } + liftListItem(listItemType)(state, dispatch); + if (view) { + wrapInList(bulletListType)(view.state, dispatch); + const newCtx = findListContext(view.state); + if (newCtx) { + dispatch(addChecksToSelection(view.state, newCtx)); + } + } + return true; + } + + if (!wrapInList) { + return false; + } + if (!dispatch) { + return wrapInList(bulletListType)(state, undefined); + } + + wrapInList(bulletListType)(state, dispatch); + if (view) { + const newCtx = findListContext(view.state); + if (newCtx) { + dispatch(addChecksToSelection(view.state, newCtx)); + } + } + return true; + }; + }, + }; + }, + + state: ({ schema, utils: { inNode } }, viewState) => { + const { $from } = viewState.selection; + let inCheckList = false; + + for (let depth = $from.depth; depth > 0; depth--) { + const node = $from.node(depth); + if (node.type === schema.nodes.list_item) { + if ($from.node(depth - 1)?.type === schema.nodes.bullet_list) { + const p = node.firstChild; + inCheckList = + p?.isTextblock && p.firstChild?.type === schema.nodes.check; + } + break; + } + } + + return { + inCheckList, + inBulletList: !inCheckList && inNode(viewState, schema.nodes.bullet_list), + }; + }, }; const CHECKED_REGEX = /\bchecked\b/; diff --git a/plugins/checklist/assets/stylesheets/checklist.scss b/plugins/checklist/assets/stylesheets/checklist.scss index 238a9e0e66ab2..a1519e93416af 100644 --- a/plugins/checklist/assets/stylesheets/checklist.scss +++ b/plugins/checklist/assets/stylesheets/checklist.scss @@ -83,8 +83,7 @@ ul li.has-checkbox { } .ProseMirror { - // TODO(renato): this is temporary, we should use `has-checkbox` instead - li:has(p:first-of-type > span.chcklst-box) { + ul > li.has-checkbox { list-style-type: none; .chcklst-box:first-of-type { diff --git a/plugins/checklist/config/locales/client.en.yml b/plugins/checklist/config/locales/client.en.yml new file mode 100644 index 0000000000000..4c5f683d2d1ec --- /dev/null +++ b/plugins/checklist/config/locales/client.en.yml @@ -0,0 +1,5 @@ +en: + js: + checklist: + composer: + checklist: "Check list" diff --git a/plugins/checklist/spec/system/page_objects/components/rich_checklist.rb b/plugins/checklist/spec/system/page_objects/components/rich_checklist.rb new file mode 100644 index 0000000000000..88c0f82e2aac2 --- /dev/null +++ b/plugins/checklist/spec/system/page_objects/components/rich_checklist.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module PageObjects + module Components + class RichChecklist < PageObjects::Components::Base + CHECKBOX_SELECTOR = ".chcklst-box" + UNCHECKED_SELECTOR = "#{CHECKBOX_SELECTOR}.fa.fa-square-o" + CHECKED_SELECTOR = "#{CHECKBOX_SELECTOR}.checked.fa.fa-square-check-o" + + def initialize(rich_editor) + @rich_editor = rich_editor + end + + def click_checkbox(index = 0) + @rich_editor.find_all(CHECKBOX_SELECTOR)[index].click + self + end + + def has_checkboxes?(count:) + @rich_editor.has_css?(CHECKBOX_SELECTOR, count: count) + end + + def has_no_checkboxes? + @rich_editor.has_no_css?(CHECKBOX_SELECTOR) + end + + def has_checked?(count: 1) + @rich_editor.has_css?(CHECKED_SELECTOR, count: count) + end + + def has_unchecked?(count: 1) + @rich_editor.has_css?(UNCHECKED_SELECTOR, count: count) + end + + def has_items?(count:) + @rich_editor.has_css?("ul li", count: count) + end + end + end +end diff --git a/plugins/checklist/spec/system/rich_editor_extension_spec.rb b/plugins/checklist/spec/system/rich_editor_extension_spec.rb new file mode 100644 index 0000000000000..2b5f4171e246b --- /dev/null +++ b/plugins/checklist/spec/system/rich_editor_extension_spec.rb @@ -0,0 +1,141 @@ +# frozen_string_literal: true + +describe "Composer - ProseMirror editor - Checklist extension", type: :system do + fab!(:user) { Fabricate(:user, refresh_auto_groups: true) } + let(:composer) { PageObjects::Components::Composer.new } + let(:rich) { composer.rich_editor } + let(:checklist) { PageObjects::Components::RichChecklist.new(rich) } + + before do + sign_in(user) + SiteSetting.rich_editor = true + end + + def open_composer_and_toggle_rich_editor + page.visit "/new-topic" + expect(composer).to be_opened + composer.toggle_rich_editor + end + + def click_checklist_toolbar_option + find(".toolbar__button.list").click + find("button[data-name='list-checklist']").click + end + + describe "checklist functionality" do + it "toggles checked state when clicking checkbox" do + open_composer_and_toggle_rich_editor + rich.click + + rich.send_keys("[ ] Item 1") + expect(checklist).to have_unchecked(count: 1) + + checklist.click_checkbox + expect(checklist).to have_checked(count: 1) + end + + it "shows checklist option in toolbar" do + open_composer_and_toggle_rich_editor + rich.click + + find(".toolbar__button.list").click + + expect(page).to have_css("button[data-name='list-checklist']") + end + end + + describe "checklist structure" do + it "creates proper list structure with toolbar" do + open_composer_and_toggle_rich_editor + rich.click + + click_checklist_toolbar_option + rich.send_keys("First item") + + expect(checklist).to have_items(count: 1) + expect(checklist).to have_checkboxes(count: 1) + end + + it "continues checklist on Enter" do + open_composer_and_toggle_rich_editor + rich.click + + click_checklist_toolbar_option + rich.send_keys("First item") + rich.send_keys(:enter) + + expect(checklist).to have_items(count: 2) + expect(checklist).to have_checkboxes(count: 2) + end + + it "allows double-Enter to escape checklist" do + open_composer_and_toggle_rich_editor + rich.click + + click_checklist_toolbar_option + rich.send_keys("First item") + rich.send_keys(:enter) + + expect(checklist).to have_checkboxes(count: 2) + + rich.send_keys(:enter) + + expect(checklist).to have_checkboxes(count: 1) + expect(checklist).to have_items(count: 1) + end + end + + describe "backspace behavior" do + it "joins with previous item when backspacing at start of checklist item with content" do + open_composer_and_toggle_rich_editor + rich.click + + click_checklist_toolbar_option + rich.send_keys("First item") + rich.send_keys(:enter) + rich.send_keys("Second item") + + expect(checklist).to have_checkboxes(count: 2) + expect(checklist).to have_items(count: 2) + + rich.send_keys(:home) + rich.send_keys(:backspace) + + expect(checklist).to have_checkboxes(count: 1) + expect(rich).to have_text("First itemSecond item") + end + + it "joins with previous item when backspacing on empty checklist item" do + open_composer_and_toggle_rich_editor + rich.click + + click_checklist_toolbar_option + rich.send_keys("First item") + rich.send_keys(:enter) + + expect(checklist).to have_checkboxes(count: 2) + + rich.send_keys(:backspace) + + expect(checklist).to have_checkboxes(count: 1) + expect(rich).to have_text("First item") + end + + it "removes checkbox when backspacing on first/only checklist item" do + open_composer_and_toggle_rich_editor + rich.click + + click_checklist_toolbar_option + rich.send_keys("Only item") + + expect(checklist).to have_checkboxes(count: 1) + expect(checklist).to have_items(count: 1) + + rich.send_keys(:home) + rich.send_keys(:backspace) + + expect(checklist).to have_no_checkboxes + expect(rich).to have_text("Only item") + end + end +end diff --git a/plugins/checklist/test/javascripts/integration/rich-editor-extension-test.js b/plugins/checklist/test/javascripts/integration/rich-editor-extension-test.js index 505316932fd02..d2c95174f6c33 100644 --- a/plugins/checklist/test/javascripts/integration/rich-editor-extension-test.js +++ b/plugins/checklist/test/javascripts/integration/rich-editor-extension-test.js @@ -43,7 +43,7 @@ module( ], "handles checkboxes in lists": [ "* [ ] unchecked list item\n* [x] checked list item", - ``, + ``, "* [ ] unchecked list item\n* [x] checked list item", ], "handles checkboxes with formatting": [ @@ -51,21 +51,26 @@ module( `

    ${unchecked} italics and ${checked} bold

    `, "[ ] *italics* and [x] **bold**", ], - "does not render escaped opening bracket as checkbox": [ + "does not render escaped checkbox": [ "\\[x] not a checkbox", "

    [x] not a checkbox

    ", "\\[x\\] not a checkbox", ], - "does not render escaped closing bracket as checkbox": [ - "[x\\] not a checkbox", - "

    [x] not a checkbox

    ", - "\\[x\\] not a checkbox", - ], "handles escaped checkbox followed by real checkbox": [ "\\[x] escaped [x] real", `

    [x] escaped ${checked} real

    `, "\\[x\\] escaped [x] real", ], + "preserves checkbox state in ordered lists": [ + "1. [ ] unchecked\n2. [x] checked", + `

    1. ${unchecked} unchecked

    2. ${checked} checked

    `, + "1. [ ] unchecked\n2. [x] checked", + ], + "handles nested list with checkboxes": [ + "* [ ] parent\n * [x] child", + ``, + "* [ ] parent\n * [x] child", + ], }).forEach(([name, [markdown, html, expectedMarkdown]]) => { test(name, async function (assert) { await testMarkdown(assert, markdown, html, expectedMarkdown); diff --git a/spec/system/composer/prosemirror_toolbar_spec.rb b/spec/system/composer/prosemirror_toolbar_spec.rb index 28c8c9863d0c9..adc67add78f23 100644 --- a/spec/system/composer/prosemirror_toolbar_spec.rb +++ b/spec/system/composer/prosemirror_toolbar_spec.rb @@ -11,7 +11,6 @@ expect(page).to have_css(".toolbar__button.italic.--active", count: 0) expect(page).to have_css(".toolbar__button.heading.--active", count: 0) expect(page).to have_css(".toolbar__button.link.--active", count: 0) - expect(page).to have_css(".toolbar__button.bullet.--active", count: 0) expect(page).to have_css(".toolbar__button.list.--active", count: 0) expect(page).to have_css(".toolbar__button.code.--active", count: 0) expect(page).to have_css(".toolbar__button.blockquote.--active", count: 0) @@ -22,16 +21,9 @@ expect(page).to have_css(".toolbar__button.bold.--active", count: 1) expect(page).to have_css(".toolbar__button.italic.--active", count: 1) expect(page).to have_css(".toolbar__button.link.--active", count: 1) - expect(page).to have_css(".toolbar__button.bullet.--active", count: 1) - expect(page).to have_css(".toolbar__button.list.--active", count: 0) + expect(page).to have_css(".toolbar__button.list.--active", count: 1) expect(page).to have_css(".toolbar__button.code.--active", count: 1) expect(page).to have_css(".toolbar__button.blockquote.--active", count: 1) - - page.find(".toolbar__button.bullet").click - page.find(".toolbar__button.list").click - - expect(page).to have_css(".toolbar__button.list.--active", count: 1) - expect(page).to have_css(".toolbar__button.bullet.--active", count: 0) end end From 13fd9a0497685229f9929d77e1536bc3ea789a4f Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Tue, 24 Feb 2026 14:11:21 +0100 Subject: [PATCH 0002/1051] DEV: Move chat message rebake into a service object (#38020) Previously the rebake endpoint in Chat::ChatController used before_action callbacks to look up the message before checking channel access. This ordering allowed distinguishing whether a message existed in an inaccessible channel via differing HTTP status codes (403 vs 404). This commit extracts the rebake logic into a Chat::RebakeMessage service following the Service::Base pattern. The service checks channel access before message lookup, ensuring unauthorized users always receive 403 regardless of message existence. It also removes the now-unused find_chat_message and preloaded_chat_message_query private methods from the controller. --- .../app/controllers/chat/chat_controller.rb | 53 +++-------- .../chat/app/services/chat/rebake_message.rb | 59 ++++++++++++ .../spec/requests/chat_controller_spec.rb | 94 +++++-------------- .../spec/services/chat/rebake_message_spec.rb | 85 +++++++++++++++++ 4 files changed, 184 insertions(+), 107 deletions(-) create mode 100644 plugins/chat/app/services/chat/rebake_message.rb create mode 100644 plugins/chat/spec/services/chat/rebake_message_spec.rb diff --git a/plugins/chat/app/controllers/chat/chat_controller.rb b/plugins/chat/app/controllers/chat/chat_controller.rb index 1af55e9d3c636..90fa502460438 100644 --- a/plugins/chat/app/controllers/chat/chat_controller.rb +++ b/plugins/chat/app/controllers/chat/chat_controller.rb @@ -2,12 +2,8 @@ module Chat class ChatController < ::Chat::BaseController - # Other endpoints use set_channel_and_chatable_with_access_check, but - # these endpoints require a standalone find because they need to be - # able to get deleted channels and recover them. - before_action :find_chat_message, only: %i[rebake] before_action :set_channel_and_chatable_with_access_check, - except: %i[respond set_user_chat_status dismiss_retention_reminder] + except: %i[respond set_user_chat_status dismiss_retention_reminder rebake] def respond render @@ -27,9 +23,20 @@ def react end def rebake - guardian.ensure_can_rebake_chat_message!(@message) - @message.rebake!(invalidate_oneboxes: true) - render json: success_json + Chat::RebakeMessage.call(service_params) do + on_success { render(json: success_json) } + on_failure { render(json: failed_json, status: :unprocessable_entity) } + on_model_not_found(:channel) { raise Discourse::NotFound } + on_failed_policy(:can_access_channel) { raise Discourse::InvalidAccess } + on_model_not_found(:message) { raise Discourse::NotFound } + on_failed_policy(:can_rebake) { raise Discourse::InvalidAccess } + on_failed_contract do |contract| + render( + json: failed_json.merge(errors: contract.errors.full_messages), + status: :bad_request, + ) + end + end end def set_user_chat_status @@ -70,35 +77,5 @@ def quote_messages ).generate_markdown render json: success_json.merge(markdown: markdown) end - - private - - def preloaded_chat_message_query - query = - Chat::Message - .includes(in_reply_to: [:user, chat_webhook_event: [:incoming_chat_webhook]]) - .includes(:revisions) - .includes(user: :primary_group) - .includes(chat_webhook_event: :incoming_chat_webhook) - .includes(reactions: :user) - .includes(:bookmarks) - .includes(uploads: { optimized_videos: :optimized_upload }) - .includes(chat_channel: :chatable) - .includes(:thread) - .includes(:chat_mentions) - - query = query.includes(user: :user_status) if SiteSetting.enable_user_status - - query - end - - def find_chat_message - @message = preloaded_chat_message_query.with_deleted - @message = @message.where(chat_channel_id: params[:chat_channel_id]) if params[ - :chat_channel_id - ] - @message = @message.find_by(id: params[:message_id]) - raise Discourse::NotFound unless @message - end end end diff --git a/plugins/chat/app/services/chat/rebake_message.rb b/plugins/chat/app/services/chat/rebake_message.rb new file mode 100644 index 0000000000000..afde79d3b0fd4 --- /dev/null +++ b/plugins/chat/app/services/chat/rebake_message.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +module Chat + # Service responsible for rebaking a chat message. + # + # @example + # Chat::RebakeMessage.call(params: { message_id: 2, chat_channel_id: 1 }, guardian: guardian) + # + class RebakeMessage + include Service::Base + + # @!method self.call(guardian:, params:) + # @param [Guardian] guardian + # @param [Hash] params + # @option params [Integer] :message_id + # @option params [Integer] :chat_channel_id + # @return [Service::Base::Context] + + params do + attribute :message_id, :integer + attribute :chat_channel_id, :integer + + validates :message_id, presence: true + validates :chat_channel_id, presence: true + end + + model :channel + policy :can_access_channel + model :message + policy :can_rebake + + step :rebake_message + + private + + def fetch_channel(params:) + Chat::Channel.includes(:chatable).find_by(id: params.chat_channel_id) + end + + def can_access_channel(guardian:, channel:) + guardian.can_join_chat_channel?(channel) + end + + def fetch_message(params:, channel:) + Chat::Message + .includes(chat_channel: :chatable) + .with_deleted + .find_by(id: params.message_id, chat_channel_id: channel.id) + end + + def can_rebake(guardian:, message:) + guardian.can_rebake_chat_message?(message) + end + + def rebake_message(message:) + message.rebake!(invalidate_oneboxes: true) + end + end +end diff --git a/plugins/chat/spec/requests/chat_controller_spec.rb b/plugins/chat/spec/requests/chat_controller_spec.rb index aca6fe419cfa3..58c67622df36c 100644 --- a/plugins/chat/spec/requests/chat_controller_spec.rb +++ b/plugins/chat/spec/requests/chat_controller_spec.rb @@ -33,88 +33,44 @@ def flag_message(message, flagger, flag_type: ReviewableScore.types[:off_topic]) describe "#rebake" do fab!(:chat_message) { Fabricate(:chat_message, chat_channel: chat_channel, user: user) } - context "as staff" do - it "rebakes the post" do - sign_in(Fabricate(:admin)) - - expect_enqueued_with( - job: Jobs::Chat::ProcessMessage, - args: { - chat_message_id: chat_message.id, - }, - ) do - put "/chat/#{chat_channel.id}/#{chat_message.id}/rebake.json" - - expect(response.status).to eq(200) - end + it "works" do + sign_in(admin) + put "/chat/#{chat_channel.id}/#{chat_message.id}/rebake.json" + expect(response.status).to eq(200) + end + + context "when message does not exist" do + it "returns a 404" do + sign_in(admin) + put "/chat/#{chat_channel.id}/-999/rebake.json" + expect(response.status).to eq(404) end + end - it "does not interfere with core's guardian can_rebake? for posts" do - sign_in(Fabricate(:admin)) - put "/chat/#{chat_channel.id}/#{chat_message.id}/rebake.json" - expect(response.status).to eq(200) - post = Fabricate(:post) - put "/posts/#{post.id}/rebake.json" - expect(response.status).to eq(200) + context "when channel does not exist" do + it "returns a 404" do + sign_in(admin) + put "/chat/-999/#{chat_message.id}/rebake.json" + expect(response.status).to eq(404) end + end - it "does not rebake the post when channel is read_only" do - chat_message.chat_channel.update!(status: :read_only) - sign_in(Fabricate(:admin)) + context "when user cannot access the channel" do + fab!(:inaccessible_channel, :private_category_channel) - put "/chat/#{chat_channel.id}/#{chat_message.id}/rebake.json" + it "returns a 403" do + sign_in(Fabricate(:user)) + put "/chat/#{inaccessible_channel.id}/-999/rebake.json" expect(response.status).to eq(403) end - - context "when cooked has changed" do - it "marks the message as dirty" do - sign_in(Fabricate(:admin)) - chat_message.update!(message: "new content") - - expect_enqueued_with( - job: Jobs::Chat::ProcessMessage, - args: { - chat_message_id: chat_message.id, - }, - ) do - put "/chat/#{chat_channel.id}/#{chat_message.id}/rebake.json" - - expect(response.status).to eq(200) - end - end - end end - context "when not staff" do - it "forbids non staff to rebake" do + context "when user cannot rebake" do + it "returns a 403" do sign_in(Fabricate(:user)) put "/chat/#{chat_channel.id}/#{chat_message.id}/rebake.json" expect(response.status).to eq(403) end - - context "as TL3 user" do - it "forbids less then TL4 user tries to rebake" do - sign_in(Fabricate(:user, trust_level: TrustLevel[3])) - put "/chat/#{chat_channel.id}/#{chat_message.id}/rebake.json" - expect(response.status).to eq(403) - end - end - - context "as TL4 user" do - it "allows TL4 users to rebake" do - sign_in(Fabricate(:user, trust_level: TrustLevel[4])) - put "/chat/#{chat_channel.id}/#{chat_message.id}/rebake.json" - expect(response.status).to eq(200) - end - - it "does not rebake the post when channel is read_only" do - chat_message.chat_channel.update!(status: :read_only) - sign_in(Fabricate(:user, trust_level: TrustLevel[4])) - - put "/chat/#{chat_channel.id}/#{chat_message.id}/rebake.json" - expect(response.status).to eq(403) - end - end end end diff --git a/plugins/chat/spec/services/chat/rebake_message_spec.rb b/plugins/chat/spec/services/chat/rebake_message_spec.rb new file mode 100644 index 0000000000000..bf681ef005993 --- /dev/null +++ b/plugins/chat/spec/services/chat/rebake_message_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +RSpec.describe Chat::RebakeMessage do + describe described_class::Contract, type: :model do + it { is_expected.to validate_presence_of(:message_id) } + it { is_expected.to validate_presence_of(:chat_channel_id) } + end + + describe ".call" do + subject(:result) { described_class.call(params:, **dependencies) } + + fab!(:admin) + fab!(:message, :chat_message) + + let(:guardian) { Guardian.new(admin) } + let(:params) { { message_id: message.id, chat_channel_id: message.chat_channel_id } } + let(:dependencies) { { guardian: } } + + before do + SiteSetting.chat_enabled = true + SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone] + end + + context "when params are not valid" do + let(:params) { {} } + + it { is_expected.to fail_a_contract } + end + + context "when the channel does not exist" do + let(:params) { { message_id: message.id, chat_channel_id: -1 } } + + it { is_expected.to fail_to_find_a_model(:channel) } + end + + context "when the user cannot access the channel" do + fab!(:private_channel) do + Fabricate( + :private_category_channel, + group: Fabricate(:group), + chatable: Fabricate(:private_category, group: Fabricate(:group)), + ) + end + + let(:current_user) { Fabricate(:user) } + let(:guardian) { Guardian.new(current_user) } + let(:params) { { message_id: -999, chat_channel_id: private_channel.id } } + + it { is_expected.to fail_a_policy(:can_access_channel) } + end + + context "when the message does not exist" do + let(:params) { { message_id: -1, chat_channel_id: message.chat_channel_id } } + + it { is_expected.to fail_to_find_a_model(:message) } + end + + context "when the message does not belong to the channel" do + let(:other_channel) { Fabricate(:category_channel) } + let(:params) { { message_id: message.id, chat_channel_id: other_channel.id } } + + it { is_expected.to fail_to_find_a_model(:message) } + end + + context "when the user does not have permission to rebake" do + let(:current_user) { Fabricate(:user) } + let(:guardian) { Guardian.new(current_user) } + + it { is_expected.to fail_a_policy(:can_rebake) } + end + + context "when the user has permission to rebake" do + it { is_expected.to run_successfully } + + it "enqueues a process message job" do + expect_enqueued_with( + job: Jobs::Chat::ProcessMessage, + args: { + chat_message_id: message.id, + }, + ) { result } + end + end + end +end From cf726be8d178b370263bfed04e13897e4b717c31 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Tue, 24 Feb 2026 14:11:32 +0100 Subject: [PATCH 0003/1051] FIX: Enforce allow_membership_requests setting in groups controller (#38017) The request_membership action did not check the group's allow_membership_requests setting, allowing any logged-in user who could see the group to submit a membership request. Also prevents existing group members from submitting requests. --- app/controllers/groups_controller.rb | 3 +++ spec/requests/groups_controller_spec.rb | 33 ++++++++++++++++++++----- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 257332b84106e..918d8cbcbe3d0 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -610,6 +610,9 @@ def request_membership group = find_group(:name) + raise Discourse::InvalidAccess unless group.allow_membership_requests? + raise Discourse::InvalidAccess if group.users.exists?(id: current_user.id) + begin GroupRequest.create!(group: group, user: current_user, reason: params[:reason]) rescue ActiveRecord::RecordNotUnique diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index a5be5a6682798..ab19a26536b24 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -2507,20 +2507,41 @@ def expect_type_to_return_right_groups(type, expected_group_ids) describe "#request_membership" do fab!(:new_user, :user) + before { group.update_column(:allow_membership_requests, true) } + it "requires the user to log in" do post "/groups/#{group.name}/request_membership.json" expect(response.status).to eq(403) end - it "requires a reason" do + it "rejects the request when allow_membership_requests is false" do + group.update!(allow_membership_requests: false) + sign_in(new_user) + + post "/groups/#{group.name}/request_membership.json", params: { reason: "Please add me" } + + expect(response.status).to eq(403) + expect(GroupRequest.where(group: group, user: new_user).exists?).to eq(false) + end + + it "rejects the request when the user is already a group member" do sign_in(user) + post "/groups/#{group.name}/request_membership.json", params: { reason: "Please add me" } + + expect(response.status).to eq(403) + expect(GroupRequest.where(group: group, user: user).exists?).to eq(false) + end + + it "requires a reason" do + sign_in(new_user) + post "/groups/#{group.name}/request_membership.json" expect(response.status).to eq(400) end it "checks for duplicates" do - sign_in(user) + sign_in(new_user) post "/groups/#{group.name}/request_membership.json", params: { reason: "Please add me in" } @@ -2532,7 +2553,7 @@ def expect_type_to_return_right_groups(type, expected_group_ids) end it "limits the character count of the reason" do - sign_in(user) + sign_in(new_user) post "/groups/#{group.name}/request_membership.json", params: { @@ -2550,7 +2571,7 @@ def expect_type_to_return_right_groups(type, expected_group_ids) owner2 = Fabricate(:user, last_seen_at: 1.day.ago) [owner1, owner2].each { |owner| group.add_owner(owner) } - sign_in(user) + sign_in(new_user) post "/groups/#{group.name}/request_membership.json", params: { reason: "Please add me in" } @@ -2562,7 +2583,7 @@ def expect_type_to_return_right_groups(type, expected_group_ids) expect(body["relative_url"]).to eq(topic.relative_url) expect(post.topic.custom_fields["requested_group_id"].to_i).to eq(group.id) - expect(post.user).to eq(user) + expect(post.user).to eq(new_user) expect(topic.title).to eq( I18n.t("groups.request_membership_pm.title", group_name: group.name), @@ -2570,7 +2591,7 @@ def expect_type_to_return_right_groups(type, expected_group_ids) expect(post.raw).to start_with("Please add me in") expect(topic.archetype).to eq(Archetype.private_message) - expect(topic.allowed_users).to contain_exactly(user, owner1, owner2) + expect(topic.allowed_users).to contain_exactly(new_user, owner1, owner2) expect(topic.allowed_groups).to eq([]) end end From 867a59a1bd7420eac8f81c23359f02de2301a8b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Tue, 24 Feb 2026 14:27:50 +0100 Subject: [PATCH 0004/1051] FIX: Exclude notify_moderators PMs from personal message rate limit (#38021) When a user flags a post with "Something Else", Discourse creates a private message to moderators containing the flag details. This PM is created with the flagger as the topic owner, which causes it to count against the user's `max_personal_messages_per_day` rate limit. This means users who have exhausted their daily PM quota cannot flag posts with the "Something Else" reason, even though the flag itself has its own independent rate limits (`max_flags_per_day` with trust level multipliers). The error message ("You've reached the maximum messages allowed per day") is also confusing in a flagging context. This fix excludes topics with the `notify_moderators` subtype from the PM per-day rate limit check in `Topic#limit_private_messages_per_day`. Flag-generated PMs are system-initiated side effects, not user-initiated messages, and should not count against the personal message quota. https://meta.discourse.org/t/257002 --- app/models/topic.rb | 1 + spec/lib/post_action_creator_spec.rb | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/app/models/topic.rb b/app/models/topic.rb index a723c64d4ce12..7995d3ca58de2 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -520,6 +520,7 @@ def limit_topics_per_day def limit_private_messages_per_day return unless private_message? + return if subtype == TopicSubtype.notify_moderators apply_per_day_rate_limit_for("pms", :max_personal_messages_per_day) end diff --git a/spec/lib/post_action_creator_spec.rb b/spec/lib/post_action_creator_spec.rb index 44a07de66291d..a3c4be90442d9 100644 --- a/spec/lib/post_action_creator_spec.rb +++ b/spec/lib/post_action_creator_spec.rb @@ -17,6 +17,27 @@ expect { PostActionCreator.like(user, post) }.to raise_error(RateLimiter::LimitExceeded) end + + it "does not count notify_moderators PM against personal message rate limit" do + SiteSetting.max_personal_messages_per_day = 1 + SiteSetting.max_topics_per_day = 0 + SiteSetting.max_topics_in_first_day = 0 + SiteSetting.rate_limit_create_topic = 0 + SiteSetting.rate_limit_create_post = 0 + SiteSetting.rate_limit_new_user_create_post = 0 + + archetype = Archetype.private_message + + create_post(user:, archetype:, target_usernames: [admin.username]) + + expect { create_post(user:, archetype:, target_usernames: [admin.username]) }.to raise_error( + RateLimiter::LimitExceeded, + ) + + reason = "This is a 'something else' flag." + result = PostActionCreator.notify_moderators(user, post, reason) + expect(result).to be_success + end end describe "messaging" do From be1231fbb1d2f78bf2b07b67bd02f9dc5d1d278c Mon Sep 17 00:00:00 2001 From: Jarek Radosz Date: Tue, 24 Feb 2026 14:37:54 +0100 Subject: [PATCH 0005/1051] DEV: Move admin data-explorer code to the admin bundle (#38022) --- .../templates/admin-plugins/show/explorer/details.gjs | 8 ++++---- .../templates/admin-plugins/show/explorer/index.gjs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) rename plugins/discourse-data-explorer/assets/javascripts/{discourse => admin}/templates/admin-plugins/show/explorer/details.gjs (95%) rename plugins/discourse-data-explorer/assets/javascripts/{discourse => admin}/templates/admin-plugins/show/explorer/index.gjs (98%) diff --git a/plugins/discourse-data-explorer/assets/javascripts/discourse/templates/admin-plugins/show/explorer/details.gjs b/plugins/discourse-data-explorer/assets/javascripts/admin/templates/admin-plugins/show/explorer/details.gjs similarity index 95% rename from plugins/discourse-data-explorer/assets/javascripts/discourse/templates/admin-plugins/show/explorer/details.gjs rename to plugins/discourse-data-explorer/assets/javascripts/admin/templates/admin-plugins/show/explorer/details.gjs index f1e325de7ccf1..231004485aa1c 100644 --- a/plugins/discourse-data-explorer/assets/javascripts/discourse/templates/admin-plugins/show/explorer/details.gjs +++ b/plugins/discourse-data-explorer/assets/javascripts/admin/templates/admin-plugins/show/explorer/details.gjs @@ -12,10 +12,10 @@ import icon from "discourse/helpers/d-icon"; import draggable from "discourse/modifiers/draggable"; import MultiSelect from "discourse/select-kit/components/multi-select"; import { i18n } from "discourse-i18n"; -import CodeView from "../../../../components/code-view"; -import ExplorerSchema from "../../../../components/explorer-schema"; -import ParamInputForm from "../../../../components/param-input-form"; -import QueryResultsWrapper from "../../../../components/query-results-wrapper"; +import CodeView from "discourse/plugins/discourse-data-explorer/discourse/components/code-view"; +import ExplorerSchema from "discourse/plugins/discourse-data-explorer/discourse/components/explorer-schema"; +import ParamInputForm from "discourse/plugins/discourse-data-explorer/discourse/components/param-input-form"; +import QueryResultsWrapper from "discourse/plugins/discourse-data-explorer/discourse/components/query-results-wrapper"; export default class QueriesDetails extends Component { get showDestroyQuery() { diff --git a/plugins/discourse-data-explorer/assets/javascripts/discourse/templates/admin-plugins/show/explorer/index.gjs b/plugins/discourse-data-explorer/assets/javascripts/admin/templates/admin-plugins/show/explorer/index.gjs similarity index 98% rename from plugins/discourse-data-explorer/assets/javascripts/discourse/templates/admin-plugins/show/explorer/index.gjs rename to plugins/discourse-data-explorer/assets/javascripts/admin/templates/admin-plugins/show/explorer/index.gjs index f8536b8c52a5f..be9ffb29918a2 100644 --- a/plugins/discourse-data-explorer/assets/javascripts/discourse/templates/admin-plugins/show/explorer/index.gjs +++ b/plugins/discourse-data-explorer/assets/javascripts/admin/templates/admin-plugins/show/explorer/index.gjs @@ -11,7 +11,7 @@ import ageWithTooltip from "discourse/helpers/age-with-tooltip"; import icon from "discourse/helpers/d-icon"; import { not } from "discourse/truth-helpers"; import { i18n } from "discourse-i18n"; -import ShareReport from "../../../../components/share-report"; +import ShareReport from "discourse/plugins/discourse-data-explorer/discourse/components/share-report"; export default