mirror of
https://github.com/wekan/wekan.git
synced 2026-11-05 01:20:07 -05:00
Card More and multi-selection list every board again for a user without a templates board.
The browser suite failed "Card -> More lists the other parents and removes
one" in 66 of 67 locales. Two faults, traced in the running app:
The board list was empty. Both helpers filtered
`_id: { $ne: currentUser.getTemplatesBoardId() }`, and that id is undefined
for a user without a templates board. The client ReactiveCache keys each
query by EJSON.stringify(selector) and runs the PARSED key, which drops
undefined: the query became `_id: {}` and matched no board. So "Change
card's parent -> Source board" and the multi-selection destination picker
offered nothing to such a user. The templates board is now excluded only
when there is one. (dialogWithBoardSwimlaneList queries Boards directly and
was not affected.)
The spec opened More from document.body, without the card behind it, and
straight after writing the parents to the database, before this browser had
received parentId. It now opens More from the card's menu as a member does,
waits until the client has the primary parent, and asserts the board and
the parent card are offered.
tests/reactiveCacheUndefinedSelector.test.cjs shows the EJSON round trip,
pins both helpers, and fails on any ReactiveCache selector that puts a
call such as getTemplatesBoardId() under $ne (it flags the previous
cardDetails.js). Run against the bundle built before this change, the spec
now fails only on the board list - the bug fixed here.
Thanks to xet7 !
This commit is contained in:
1 parent
70d4ffc8f4
commit
9ae7388a7e
4 files changed
+89
-6
No files matched your search
@@ -2695,11 +2695,17 @@ Template.cardMorePopup.onCreated(function () {
|
||||
|
||||
Template.cardMorePopup.helpers({
|
||||
boards() {
|
||||
// A user without a templates board has an undefined id here, and
|
||||
// ReactiveCache keys - and re-reads - its selector through
|
||||
// EJSON.stringify, which drops undefined: `{ $ne: undefined }` became
|
||||
// `_id: {}` and matched NO board, so the list was empty. Exclude the
|
||||
// templates board only when there is one.
|
||||
const templatesBoardId = ReactiveCache.getCurrentUser()?.getTemplatesBoardId();
|
||||
const ret = ReactiveCache.getBoards(
|
||||
{
|
||||
archived: false,
|
||||
'members.userId': Meteor.userId(),
|
||||
_id: { $ne: ReactiveCache.getCurrentUser().getTemplatesBoardId() },
|
||||
...(templatesBoardId ? { _id: { $ne: templatesBoardId } } : {}),
|
||||
},
|
||||
{
|
||||
sort: { sort: 1 /* boards default sorting */ },
|
||||
|
||||
@@ -559,11 +559,17 @@ Template.selectionDestinationPicker.onCreated(function() {
|
||||
|
||||
Template.selectionDestinationPicker.helpers({
|
||||
boards() {
|
||||
// A user without a templates board has an undefined id here, and
|
||||
// ReactiveCache keys - and re-reads - its selector through
|
||||
// EJSON.stringify, which drops undefined: `{ $ne: undefined }` became
|
||||
// `_id: {}` and matched NO board, so the list was empty. Exclude the
|
||||
// templates board only when there is one.
|
||||
const templatesBoardId = ReactiveCache.getCurrentUser()?.getTemplatesBoardId();
|
||||
return ReactiveCache.getBoards(
|
||||
{
|
||||
archived: false,
|
||||
'members.userId': Meteor.userId(),
|
||||
_id: { $ne: ReactiveCache.getCurrentUser().getTemplatesBoardId() },
|
||||
...(templatesBoardId ? { _id: { $ne: templatesBoardId } } : {}),
|
||||
},
|
||||
{
|
||||
sort: { sort: 1 },
|
||||
|
||||
@@ -48,15 +48,25 @@ test(`Card → More lists the other parents and removes one in ${language}`, asy
|
||||
await bp.clickCard(board.listIds[0], 'Alpha Card');
|
||||
const details = new CardPage(page);
|
||||
await details.waitForOpen();
|
||||
await page.evaluate(() => {
|
||||
const opener = document.querySelector('.js-more') || document.body;
|
||||
Popup.open('cardMore')({ currentTarget: opener, target: opener, preventDefault() {}, stopPropagation() {} });
|
||||
});
|
||||
// The parents were written straight to the database; the client sees
|
||||
// parentIds a moment before parentId, and More reads the primary parent once,
|
||||
// when it opens. Wait until this browser has both, as a member's would.
|
||||
await expect.poll(() => page.evaluate(id => Meteor.connection._stores.cards?._getCollection().findOne(id)?.parentId, a._id)).toBe(b._id);
|
||||
// Open More the way a member does: the card's menu, then More. A popup
|
||||
// opened from document.body has no card behind it, so it cannot know the
|
||||
// parent's board and never shows the "add parent" choice checked below.
|
||||
await details.openActionsMenu();
|
||||
await details.clickAction('.js-more');
|
||||
const others = page.locator('.card-other-parents');
|
||||
await expect(others).toContainText('Gamma Card');
|
||||
await expect(others.locator(':scope > span')).toHaveText(locale['other-parent-cards'] + ':');
|
||||
await expect(page.locator('label').filter({ has: page.locator('.js-field-add-parent-card') })).toContainText(locale['add-parent-card']);
|
||||
await expect(others.locator('.js-remove-other-parent')).toHaveAttribute('title', locale['remove-parent-card']);
|
||||
// The source board list is not empty for a user without a templates board:
|
||||
// its selector used to read `_id: { $ne: undefined }`, which ReactiveCache's
|
||||
// EJSON key turned into `_id: {}` - matching no board at all.
|
||||
await expect(page.locator('.js-pop-over .js-field-parent-board option[value="' + board.boardId + '"]')).toHaveCount(1);
|
||||
await expect(page.locator('.js-pop-over .js-field-add-parent-card option[value="' + b._id + '"]')).toHaveCount(1);
|
||||
await others.locator('.js-remove-other-parent').click();
|
||||
await expect.poll(() => byTitle('Alpha Card').parentIds).toEqual([b._id]);
|
||||
expect(byTitle('Alpha Card').parentId).toBe(b._id);
|
||||
|
||||
@@ -0,0 +1,61 @@
|
||||
'use strict';
|
||||
// The client ReactiveCache keys every query by EJSON.stringify(selector) and
|
||||
// runs the query on the PARSED key (imports/reactiveCache.js, DataCache). A
|
||||
// value that is undefined disappears in that round trip, so
|
||||
// `_id: { $ne: undefined }` - "every board" - is queried as `_id: {}`, which
|
||||
// matches no document. The card More popup and the multi-selection
|
||||
// destination picker filtered out `getTemplatesBoardId()`, which is undefined
|
||||
// for a user without a templates board, and listed no board at all.
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
|
||||
const root = path.resolve(__dirname, '..');
|
||||
let passed = 0;
|
||||
function test(name, fn) { fn(); passed += 1; console.log(' ok -', name); }
|
||||
|
||||
test('the round trip ReactiveCache makes really turns $ne: undefined into {}', () => {
|
||||
// EJSON.stringify drops undefined exactly as JSON.stringify does.
|
||||
const key = JSON.stringify({ selector: { archived: false, _id: { $ne: undefined } } });
|
||||
assert.deepEqual(JSON.parse(key).selector, { archived: false, _id: {} });
|
||||
});
|
||||
|
||||
test('the two board lists exclude the templates board only when there is one', () => {
|
||||
for (const file of ['client/components/cards/cardDetails.js', 'client/components/sidebar/sidebarFilters.js']) {
|
||||
const text = fs.readFileSync(path.join(root, file), 'utf8');
|
||||
assert.match(text, /const templatesBoardId = ReactiveCache\.getCurrentUser\(\)\?\.getTemplatesBoardId\(\);/, file);
|
||||
assert.match(text, /\.\.\.\(templatesBoardId \? \{ _id: \{ \$ne: templatesBoardId \} \} : \{\}\)/, file);
|
||||
}
|
||||
});
|
||||
|
||||
function clientFiles(dir, out = []) {
|
||||
for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
|
||||
const full = path.join(dir, entry.name);
|
||||
if (entry.isDirectory()) clientFiles(full, out);
|
||||
else if (/\.js$/.test(entry.name)) out.push(full);
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
test('negative: no ReactiveCache query puts a call that may return undefined under $ne', () => {
|
||||
// getTemplatesBoardId() and friends read an optional profile field. Inside a
|
||||
// ReactiveCache selector, compute the value first and leave the clause out
|
||||
// when it is missing.
|
||||
const bad = [];
|
||||
for (const file of [...clientFiles(path.join(root, 'client')), path.join(root, 'imports/reactiveCache.js')]) {
|
||||
const text = fs.readFileSync(file, 'utf8');
|
||||
for (const call of text.matchAll(/ReactiveCache\.get\w+\(\s*\{[\s\S]*?\}\s*[,)]/g)) {
|
||||
if (/\$ne:\s*[\w.]*\([^)]*\)\??\.?\w*\(\)/.test(call[0]) || /\$ne:\s*ReactiveCache\.getCurrentUser\(\)/.test(call[0])) {
|
||||
bad.push(path.relative(root, file) + ': ' + call[0].replace(/\s+/g, ' ').slice(0, 120));
|
||||
}
|
||||
}
|
||||
}
|
||||
assert.deepEqual(bad, []);
|
||||
});
|
||||
|
||||
test('negative: the guard above does catch the shape that broke', () => {
|
||||
const broken = "ReactiveCache.getBoards({ archived: false, _id: { $ne: ReactiveCache.getCurrentUser().getTemplatesBoardId() }, }, {})";
|
||||
assert.ok(/\$ne:\s*ReactiveCache\.getCurrentUser\(\)/.test(broken));
|
||||
});
|
||||
|
||||
console.log(`\nreactiveCacheUndefinedSelector: all ${passed} tests passed`);
|
||||
Reference in new issue
Block a user