From 8521d1e45f43cdbe7a07e0e988ddb6b1f7ac868a Mon Sep 17 00:00:00 2001
From: Tom Byers <tombaromba@gmail.com>
Date: Fri, 18 Feb 2022 11:55:13 +0000
Subject: [PATCH] Add assertions against stray classes

In previous iterations of the classPersister, we
found issues with the implementation meant classes
it should have added back to elements were also
added to other elements. This adds tests for this
scenario to ensure it doesn't happen again.

Also includes changes to fix a linting error with
the JS which complained about a function being
defined in a loop while referencing variables in
the outer scope.
---
 app/assets/javascripts/updateContent.js | 19 ++++++++++---------
 tests/javascripts/updateContent.test.js |  4 ++++
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js
index b08bceaac..96e5fef33 100644
--- a/app/assets/javascripts/updateContent.js
+++ b/app/assets/javascripts/updateContent.js
@@ -38,18 +38,19 @@
     });
   };
   ClassesPersister.prototype.replace = function () {
+    var replaceClasses = (idx, el) => {
+
+      // Avoid updating elements that are no longer present.
+      // elements removed will still exist in memory but won't be attached to the DOM any more
+      if (global.document.body.contains(el)) {
+        $(el).addClass(className);
+      }
+
+    };
     var className;
 
     for (className in this._classesTo$ElsMap) {
-      this._classesTo$ElsMap[className].each((idx, el) => {
-
-        // Avoid updating elements that are no longer present.
-        // elements removed will still exist in memory but won't be attached to the DOM any more
-        if (global.document.body.contains(el)) {
-          $(el).addClass(className);
-        }
-
-      });
+      this._classesTo$ElsMap[className].each(replaceClasses);
     }
 
     // remove references to elements
diff --git a/tests/javascripts/updateContent.test.js b/tests/javascripts/updateContent.test.js
index 7c5492d7e..557f1799e 100644
--- a/tests/javascripts/updateContent.test.js
+++ b/tests/javascripts/updateContent.test.js
@@ -491,6 +491,10 @@ describe('Update content', () => {
       expect(partialsInPage[0].querySelectorAll('.file-list h2')[0].classList.contains('js-child-has-focus')).toBe(true);
       expect(partialsInPage[1].querySelectorAll('.file-list h2')[0].classList.contains('js-2nd-child-has-focus')).toBe(true);
 
+      // check each heading only has the classes assigned to it before updates occurred
+      expect(partialsInPage[0].querySelectorAll('.file-list h2')[0].classList.contains('js-2nd-child-has-focus')).toBe(false);
+      expect(partialsInPage[1].querySelectorAll('.file-list h2')[0].classList.contains('js-child-has-focus')).toBe(false);
+
     });
 
   });