mirror of
https://github.com/ether/etherpad-lite.git
synced 2026-07-17 16:47:05 +00:00
feat(settings): enable Pad-wide Settings by default; fix misleading modal title (#7679)
* feat(settings): enable Pad-wide Settings by default; fix misleading modal title The creator-owned Pad-wide Settings feature (#7545) shipped behind a flag that defaulted to false. With the flag off the modal still rendered an H1 of `pad.settings.padSettings` ("Pad-wide Settings") for *every* user, even though no pad-wide controls were ever shown. Two readers in different browsers both saw "Pad-wide Settings" as the modal title, which looked like a creator-gate regression but was just a copy bug. Two changes: 1. Flip the default of `enablePadWideSettings` to `true` (Settings.ts plus both settings templates). With the feature on, the creator (revision-0 author) gets a real "Pad-wide Settings" section gated by `clientVars.canEditPadSettings`, while every other user sees only "User Settings" — matching the design intent of #7545. This is a behavior change, so the settings comments are expanded to describe what the toggle now does. 2. Drop the conditional H1 in `src/templates/pad.html` and always use `pad.settings.title` ("Settings"). Operators who explicitly disable the feature shouldn't see a label that lies about a section that isn't rendered. Adds backend regression coverage in `tests/backend/specs/socketio.ts`: - Different browsers (different cookie jars => different authorIDs): only the first joiner gets `canEditPadSettings: true`. - Same browser, two tabs (shared HttpOnly token cookie => same authorID): both connections are the same identity, both correctly land on the creator path. * test(settings): regression coverage for the settings modal H1 Asserts the rendered `/p/<id>` HTML always uses `data-l10n-id="pad.settings.title"` for the modal heading, regardless of `enablePadWideSettings`. Catches a re-introduction of the old conditional that printed "Pad-wide Settings" for every user when the feature was off. Action of Qodo review feedback on PR #7679.
This commit is contained in:
parent
da87a4fca2
commit
b751fd7b9e
6 changed files with 117 additions and 10 deletions
|
|
@ -247,9 +247,12 @@
|
|||
|
||||
/**
|
||||
* Enable creator-owned Pad-wide Settings and new-pad default seeding from My View.
|
||||
* Disabled by default to preserve the legacy single-settings behavior.
|
||||
* The pad creator (revision-0 author) gets the "Pad-wide Settings" section,
|
||||
* which lets them set defaults and optionally enforce them for other users.
|
||||
* Other users see only "User Settings" (their own view options).
|
||||
* Set to false to revert to the legacy single-settings behavior.
|
||||
**/
|
||||
"enablePadWideSettings": "${ENABLE_PAD_WIDE_SETTINGS:false}",
|
||||
"enablePadWideSettings": "${ENABLE_PAD_WIDE_SETTINGS:true}",
|
||||
|
||||
/*
|
||||
* Optional privacy banner. See settings.json.template for full field docs.
|
||||
|
|
|
|||
|
|
@ -733,9 +733,12 @@
|
|||
|
||||
/**
|
||||
* Enable creator-owned Pad-wide Settings and new-pad default seeding from My View.
|
||||
* Disabled by default to preserve the legacy single-settings behavior.
|
||||
* The pad creator (revision-0 author) gets the "Pad-wide Settings" section,
|
||||
* which lets them set defaults and optionally enforce them for other users.
|
||||
* Other users see only "User Settings" (their own view options).
|
||||
* Set to false to revert to the legacy single-settings behavior.
|
||||
**/
|
||||
"enablePadWideSettings": "${ENABLE_PAD_WIDE_SETTINGS:false}",
|
||||
"enablePadWideSettings": "${ENABLE_PAD_WIDE_SETTINGS:true}",
|
||||
|
||||
/*
|
||||
* Optional privacy banner shown once the pad loads. Disabled by default.
|
||||
|
|
|
|||
|
|
@ -369,7 +369,7 @@ const settings: SettingsType = {
|
|||
},
|
||||
updateServer: "https://static.etherpad.org",
|
||||
enableDarkMode: true,
|
||||
enablePadWideSettings: false,
|
||||
enablePadWideSettings: true,
|
||||
allowPadDeletionByAllUsers: false,
|
||||
privacyBanner: {
|
||||
enabled: false,
|
||||
|
|
|
|||
|
|
@ -127,11 +127,7 @@
|
|||
<!------------------------------------------------------------->
|
||||
|
||||
<div id="settings" class="popup" role="dialog" aria-modal="true" aria-labelledby="settings-title"><div class="popup-content">
|
||||
<% if (settings.enablePadWideSettings) { %>
|
||||
<h1 id="settings-title" data-l10n-id="pad.settings.title">Settings</h1>
|
||||
<% } else { %>
|
||||
<h1 id="settings-title" data-l10n-id="pad.settings.padSettings">Pad Settings</h1>
|
||||
<% } %>
|
||||
<div class="settings-sections">
|
||||
<div id="user-settings-section" class="settings-section">
|
||||
<% e.begin_block("mySettings"); %>
|
||||
|
|
|
|||
45
src/tests/backend/specs/settingsModalHeading.ts
Normal file
45
src/tests/backend/specs/settingsModalHeading.ts
Normal file
|
|
@ -0,0 +1,45 @@
|
|||
'use strict';
|
||||
|
||||
import {MapArrayType} from '../../../node/types/MapType';
|
||||
import settings from '../../../node/utils/Settings';
|
||||
|
||||
const assert = require('assert').strict;
|
||||
const common = require('../common');
|
||||
|
||||
// Regression coverage for the settings modal title. With
|
||||
// `enablePadWideSettings: false` the template used to render
|
||||
// `data-l10n-id="pad.settings.padSettings"` ("Pad-wide Settings") for every
|
||||
// user, even though no pad-wide controls were rendered in that mode. The fix
|
||||
// removes the conditional and always uses `pad.settings.title` ("Settings").
|
||||
describe(__filename, function () {
|
||||
this.timeout(30000);
|
||||
let agent: any;
|
||||
const backup: MapArrayType<any> = {};
|
||||
|
||||
before(async function () { agent = await common.init(); });
|
||||
|
||||
beforeEach(async function () {
|
||||
backup.enablePadWideSettings = settings.enablePadWideSettings;
|
||||
});
|
||||
|
||||
afterEach(async function () {
|
||||
settings.enablePadWideSettings = backup.enablePadWideSettings;
|
||||
});
|
||||
|
||||
const titleH1 = (html: string): string | null => {
|
||||
const m = html.match(/<h1\s+id="settings-title"[^>]*data-l10n-id="([^"]+)"/);
|
||||
return m ? m[1] : null;
|
||||
};
|
||||
|
||||
it('uses pad.settings.title with the feature enabled', async function () {
|
||||
settings.enablePadWideSettings = true;
|
||||
const res = await agent.get('/p/headingTest').expect(200);
|
||||
assert.equal(titleH1(res.text), 'pad.settings.title');
|
||||
});
|
||||
|
||||
it('uses pad.settings.title with the feature disabled (no misleading "Pad-wide" label)', async function () {
|
||||
settings.enablePadWideSettings = false;
|
||||
const res = await agent.get('/p/headingTest').expect(200);
|
||||
assert.equal(titleH1(res.text), 'pad.settings.title');
|
||||
});
|
||||
});
|
||||
|
|
@ -34,7 +34,7 @@ describe(__filename, function () {
|
|||
plugins.hooks[hookName] = [];
|
||||
}
|
||||
backups.settings = {};
|
||||
for (const setting of ['editOnly', 'requireAuthentication', 'requireAuthorization', 'users']) {
|
||||
for (const setting of ['editOnly', 'requireAuthentication', 'requireAuthorization', 'users', 'enablePadWideSettings']) {
|
||||
// @ts-ignore
|
||||
backups.settings[setting] = settings[setting];
|
||||
}
|
||||
|
|
@ -432,6 +432,66 @@ describe(__filename, function () {
|
|||
});
|
||||
});
|
||||
|
||||
describe('Pad-wide settings creator gate', function () {
|
||||
let socketA: any;
|
||||
let socketB: any;
|
||||
|
||||
const removeIfExists = async (padId: string) => {
|
||||
if (await padManager.doesPadExist(padId)) {
|
||||
const p = await padManager.getPad(padId);
|
||||
await p.remove();
|
||||
}
|
||||
};
|
||||
|
||||
beforeEach(async function () {
|
||||
// @ts-ignore - test toggles a public setting
|
||||
settings.enablePadWideSettings = true;
|
||||
await removeIfExists('foo');
|
||||
});
|
||||
|
||||
afterEach(async function () {
|
||||
for (const s of [socketA, socketB]) if (s) s.close();
|
||||
socketA = null;
|
||||
socketB = null;
|
||||
socket = null;
|
||||
await removeIfExists('foo');
|
||||
});
|
||||
|
||||
it('different browsers (separate cookie jars): only the creator gets canEditPadSettings', async function () {
|
||||
const supertest = require('supertest');
|
||||
const browserA = supertest(common.baseUrl);
|
||||
const browserB = supertest(common.baseUrl);
|
||||
|
||||
const resA = await browserA.get('/p/foo').expect(200);
|
||||
socketA = await common.connect(resA);
|
||||
const cvA = await common.handshake(socketA, 'foo');
|
||||
assert.equal(cvA.data.canEditPadSettings, true,
|
||||
'first joiner (creator) should see Pad-wide Settings');
|
||||
|
||||
const resB = await browserB.get('/p/foo').expect(200);
|
||||
socketB = await common.connect(resB);
|
||||
const cvB = await common.handshake(socketB, 'foo');
|
||||
assert.equal(cvB.data.canEditPadSettings, false,
|
||||
'non-creator joiner must NOT see Pad-wide Settings');
|
||||
});
|
||||
|
||||
it('same browser two tabs (shared cookie jar): BOTH get canEditPadSettings=true', async function () {
|
||||
// Reusing the same response (and its set-cookie header) for both
|
||||
// connects is the backend equivalent of two browser tabs sharing the
|
||||
// same HttpOnly token cookie — same authorID, same creator.
|
||||
const res = await agent.get('/p/foo').expect(200);
|
||||
|
||||
socketA = await common.connect(res);
|
||||
const cvA = await common.handshake(socketA, 'foo');
|
||||
assert.equal(cvA.data.canEditPadSettings, true);
|
||||
|
||||
socketB = await common.connect(res);
|
||||
const cvB = await common.handshake(socketB, 'foo');
|
||||
assert.equal(cvB.data.canEditPadSettings, true,
|
||||
'same author across tabs is one identity, both are the creator');
|
||||
});
|
||||
});
|
||||
|
||||
describe('SocketIORouter.js', function () {
|
||||
const Module = class {
|
||||
setSocketIO(io:any) {}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue