From 42978c3c84c50a422043024c21b7165b7a30a0e1 Mon Sep 17 00:00:00 2001 From: Nolan Lawson Date: Tue, 11 Dec 2018 22:06:50 -0800 Subject: [PATCH] fix: fix duplicate statuses in threads (#783) fixes #511 --- .../_actions/addStatusOrNotification.js | 32 ++++-- tests/spec/122-replies-in-thread.js | 108 ++++++++++++++++++ tests/utils.js | 6 + 3 files changed, 137 insertions(+), 9 deletions(-) create mode 100644 tests/spec/122-replies-in-thread.js diff --git a/src/routes/_actions/addStatusOrNotification.js b/src/routes/_actions/addStatusOrNotification.js index 9c717d71..3cbe7977 100644 --- a/src/routes/_actions/addStatusOrNotification.js +++ b/src/routes/_actions/addStatusOrNotification.js @@ -39,25 +39,39 @@ async function insertUpdatesIntoTimeline (instanceName, timelineName, updates) { } } +function isValidStatusForThread (thread, timelineName, itemIdsToAdd) { + let focusedStatusId = timelineName.split('/')[1] // e.g. "status/123456" + let focusedStatusIdx = thread.indexOf(focusedStatusId) + return status => { + let repliedToStatusIdx = thread.indexOf(status.in_reply_to_id) + return ( + // A reply to an ancestor status is not valid for this thread, but for the focused status + // itself or any of its descendents, it is valid. + repliedToStatusIdx >= focusedStatusIdx && + // Not a duplicate + !thread.includes(status.id) && + // Not already about to be added + !itemIdsToAdd.includes(status.id) + ) + } +} + async function insertUpdatesIntoThreads (instanceName, updates) { if (!updates.length) { return } let threads = store.getThreads(instanceName) - - for (let timelineName of Object.keys(threads)) { + let timelineNames = Object.keys(threads) + for (let timelineName of timelineNames) { let thread = threads[timelineName] + let itemIdsToAdd = store.getForTimeline(instanceName, timelineName, 'itemIdsToAdd') || [] - let updatesForThisThread = updates.filter(status => ( - thread.includes(status.in_reply_to_id) && - !thread.includes(status.id) && - !itemIdsToAdd.includes(status.id) - )) - if (!updatesForThisThread.length) { + let validUpdates = updates.filter(isValidStatusForThread(thread, timelineName, itemIdsToAdd)) + if (!validUpdates.length) { continue } - let newItemIdsToAdd = uniq(concat(itemIdsToAdd, updatesForThisThread.map(_ => _.id))) + let newItemIdsToAdd = uniq(concat(itemIdsToAdd, validUpdates.map(_ => _.id))) if (!isEqual(itemIdsToAdd, newItemIdsToAdd)) { console.log('adding ', (newItemIdsToAdd.length - itemIdsToAdd.length), 'items to itemIdsToAdd for thread', timelineName) diff --git a/tests/spec/122-replies-in-thread.js b/tests/spec/122-replies-in-thread.js new file mode 100644 index 00000000..d1891897 --- /dev/null +++ b/tests/spec/122-replies-in-thread.js @@ -0,0 +1,108 @@ +import { loginAsFoobar } from '../roles' +import { + getNthStatus, + getNthStatusContent, + getUrl, + getNthReplyButton, getNthComposeReplyInput, sleep, + getAriaSetSize, + getNthComposeReplyButton, goBack, goForward +} from '../utils' +import { postAs, postReplyAs } from '../serverActions' + +fixture`122-replies-in-threads.js` + .page`http://localhost:4002` + +const TIMEOUT = 1500 + +async function goBackAndForward (t) { + await goBack() + await t.expect(getUrl()).eql('http://localhost:4002/') + await goForward() + await t.expect(getUrl()).match(/statuses/) +} + +async function verifyAriaSetSize (t, size) { + await sleep(TIMEOUT) + await t.expect(getAriaSetSize()).eql(size) + await goBackAndForward(t) + await sleep(TIMEOUT) + await t.expect(getAriaSetSize()).eql(size) +} + +// You click on status B, which is a reply to status A. +// Now status C comes in, which is a response to status A. +// Status C is inserted automatically, even though it shouldn't be in that thread. +test('reply to non-focused parent status in a thread', async t => { + const { id } = await postAs('admin', 'here is my awesome thread') + await postReplyAs('admin', 'and here it is continued', id) + await loginAsFoobar(t) + await t + .hover(getNthStatus(0)) + .expect(getNthStatusContent(0).innerText).contains('and here it is continued') + .click(getNthStatus(0)) + .expect(getUrl()).match(/statuses/) + .click(getNthReplyButton(0)) + .typeText(getNthComposeReplyInput(0), 'haha I totally agree', { paste: true }) + .click(getNthComposeReplyButton(0)) + await verifyAriaSetSize(t, '2') +}) + +// In this case it's the same as the previous, except the focused status is status A. +test('reply to focused status in a thread', async t => { + const { id } = await postAs('admin', 'whoa here is my other awesome thread') + await postReplyAs('admin', 'whoa and here it is probably continued', id) + await loginAsFoobar(t) + await t + .hover(getNthStatus(0)) + .expect(getNthStatusContent(0).innerText).contains('whoa and here it is probably continued') + .hover(getNthStatusContent(1)) + .click(getNthStatus(1)) + .expect(getUrl()).match(/statuses/) + .click(getNthReplyButton(0)) + .typeText(getNthComposeReplyInput(0), 'haha I totally agree', { paste: true }) + .click(getNthComposeReplyButton(0)) + await verifyAriaSetSize(t, '3') +}) + +// In this case the thread is A-B-C, the focused status is C, +// and the replied-to status is A. +test('reply to non-focused grandparent status in a thread', async t => { + const { id: id1 } = await postAs('admin', 'cool thread 1/3') + const { id: id2 } = await postReplyAs('admin', 'cool thread 2/3', id1) + await postReplyAs('admin', 'cool thread 3/3', id2) + await loginAsFoobar(t) + await t + .hover(getNthStatus(0)) + .expect(getNthStatusContent(0).innerText).contains('cool thread 3/3') + .click(getNthStatus(0)) + .expect(getUrl()).match(/statuses/) + .hover(getNthStatus(2)) + .hover(getNthStatus(1)) + .hover(getNthStatus(0)) + .click(getNthReplyButton(0)) + .typeText(getNthComposeReplyInput(0), 'this is sweet', { paste: true }) + .click(getNthComposeReplyButton(0)) + await verifyAriaSetSize(t, '3') +}) + +// Thread is A-B-C, focused status is A, replied-to status is C. +test('reply to non-focused grandchild status in a thread', async t => { + const { id: id1 } = await postAs('admin', 'sweet thread 1/3') + const { id: id2 } = await postReplyAs('admin', 'sweet thread 2/3', id1) + await postReplyAs('admin', 'sweet thread 3/3', id2) + await loginAsFoobar(t) + await t + .hover(getNthStatus(0)) + .expect(getNthStatusContent(0).innerText).contains('sweet thread 3/3') + .hover(getNthStatusContent(1)) + .hover(getNthStatus(2)) + .click(getNthStatus(2)) + .expect(getUrl()).match(/statuses/) + .hover(getNthStatus(0)) + .hover(getNthStatus(1)) + .hover(getNthStatus(2)) + .click(getNthReplyButton(2)) + .typeText(getNthComposeReplyInput(2), 'this is sweet', { paste: true }) + .click(getNthComposeReplyButton(2)) + await verifyAriaSetSize(t, '4') +}) diff --git a/tests/utils.js b/tests/utils.js index 23139970..45153a24 100644 --- a/tests/utils.js +++ b/tests/utils.js @@ -100,6 +100,8 @@ export const getTitleText = exec(() => document.head.querySelector('title').inne export const goBack = exec(() => window.history.back()) +export const goForward = exec(() => window.history.forward()) + export const forceOffline = exec(() => window.__forceOnline(false)) export const forceOnline = exec(() => window.__forceOnline(true)) @@ -181,6 +183,10 @@ export function getNthDeleteMediaButton (n) { return $(`.compose-media:nth-child(${n}) .compose-media-delete-button`) } +export function getAriaSetSize () { + return getNthStatus(0).getAttribute('aria-setsize') +} + export function getNthStatus (n) { return $(getNthStatusSelector(n)) }