From 420092809495ef83b83f6a802679940e0d11d1df Mon Sep 17 00:00:00 2001
From: Dennis Snell <dennis.snell@automattic.com>
Date: Tue, 8 Oct 2019 16:59:48 -0700
Subject: [PATCH] Refactor note tag operation to stop directly mutating note
 object

See #1614

As part of a broader effort to resolve data-flow issues in the app this PR is a
first step in removing direct mutation where transactional atomic updates
should be occurring.

It's not clear if the existing code is the source of existing defects in the software
and this is part of why the code is problematic; we have created inherent
concurrency flaws that open up extremely-difficult-to-reproduce bugs.

Resolving this may or may not resolve any existing bugs but it will definitely
help guard us from introducing new ones.

---

Previously we have been directly mutating note objects when editing
their tags. This mutation can lead to concurrency defects which expose
themselves as inconsistent UI state. This breaks our Redux model which
assumes that all UI updates happen atomically.

In this patch we're building new note objects when we make these updates
in order to maintain our consistency.

---

There should be no significant visual or behavioral changes with this PR. We
are changing code related to removing tags, renaming tags, and
reordering tags.

In testing verify that with separate sessions the updates appear as expected.
Add, reorder, and remove tags to make sure the changes synchronize.
---
 lib/state/domain/notes.js | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/lib/state/domain/notes.js b/lib/state/domain/notes.js
index 7f369f0f4..ff4624689 100644
--- a/lib/state/domain/notes.js
+++ b/lib/state/domain/notes.js
@@ -3,33 +3,25 @@ import appState from '../../flux/app-state';
 import isEmailTag from '../../utils/is-email-tag';
 import { createTag } from './tags';
 
-const { selectNote } = appState.actionCreators;
-
 export const updateNoteTags = ({ note, tags }) => {
   return (dispatch, getState) => {
-    if (note) {
-      let state = getState().appState;
-
-      note.data.tags = tags;
-      note.data.modificationDate = Math.floor(Date.now() / 1000);
-      noteBucket().update(note.id, note.data);
-
-      dispatch(selectNote({ note }));
-
-      let currentTagNames = state.tags.map(tag => tag.data.name);
-      for (let i = 0; i < tags.length; i++) {
-        let tag = tags[i];
+    if (!note) {
+      return;
+    }
 
-        if (currentTagNames.indexOf(tag) !== -1) {
-          continue;
-        }
+    noteBucket().update(note.id, {
+      ...note.data,
+      tags,
+      modificationDate: Math.floor(Date.now() / 1000),
+    });
 
-        if (isEmailTag(tag)) {
-          continue;
-        }
+    const existingTagNames = new Set(
+      getState().appState.tags.map(tag => tag.data.name)
+    );
 
-        createTag({ name: tag });
-      }
-    }
+    tags
+      .filter(name => !existingTagNames.has(name))
+      .filter(name => !isEmailTag(name))
+      .forEach(name => createTag({ name }));
   };
 };