Fix error causing same role assignments every day (#73)

* Fix error causing same role assignments every day A bug in the left and right filters in calculateMovesToBestAssignment() led to the algorithm ignoring all past assignments. This, in turn, led to the same role assignment every day. - Add a test case to ensure that a person who never did a role is the first choice for that role. - Fix the assertions in existing tests based on hand calculation of the Munkres algorithm input matrix. My guess is that the original assertions were derived from the broken code's output so they weren't reliable. - Reuse the `key` function instead of reimplementing it inline. * Fix "Could not find a declaration file for module 'history'"
pivotal-cf · Jun 2, 2023 · 95856f2 · 95856f2
1 parent d8a92bc
commit 95856f2
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 10 deletions.
diff --git a/package.json b/package.json
@@ -11,6 +11,7 @@
     "@iconify-icons/la": "^1.1.0",
     "@iconify/react": "^1.1.3",
     "@material-ui/core": "^4.11.2",
+    "@types/history": "^4.7.11",
     "big-integer": "^1.6.48",
     "firebase": "^8.1.1",
     "firebase-admin": "^9.4.1",

diff --git a/src/lib/recommendation.spec.ts b/src/lib/recommendation.spec.ts
@@ -848,6 +848,57 @@ describe('Recommendation', () => {
       ]);
     });
 
+    describe('when 1 pair has not done the role ever', () => {
+      it("assigns the role to that pair", () => {
+        const best = Recommendation.calculateMovesToBestAssignment({
+          left: 'person',
+          right: 'role',
+          current: {
+            entities: [
+              { '.key': 'p1', type: 'person', location: 'l1' },
+              { '.key': 'p2', type: 'person', location: 'l1' },
+              { '.key': 'p3', type: 'person', location: 'l2' },
+              { '.key': 'r1', type: 'role', location: constants.LOCATION.UNASSIGNED },
+            ],
+            lanes: [{ '.key': 'l1' }, { '.key': 'l2' }],
+          },
+          history: [
+            {
+              '.key': '' + previousScore(3),
+              entities: [],
+            },
+            {
+              '.key': '' + previousScore(2),
+              entities: [
+                { '.key': 'p1', type: 'person', location: 'l1' },
+                { '.key': 'p2', type: 'person', location: 'l2' },
+                { '.key': 'p3', type: 'person', location: 'l1' },
+                { '.key': 'r1', type: 'role', location: 'l2' },
+              ],
+            },
+            {
+              '.key': '' + previousScore(1),
+              entities: [
+                { '.key': 'p1', type: 'person', location: 'l1' },
+                { '.key': 'p2', type: 'person', location: 'l1' },
+                { '.key': 'p3', type: 'person', location: 'l2' },
+                { '.key': 'r1', type: 'role', location: 'l1' },
+              ],
+            },
+          ],
+        });
+
+        expect(best).toEqual(
+          expect.arrayContaining([
+            {
+              lane: 'l2',
+              entities: ['r1'],
+            },
+          ])
+        );
+      });
+    });
+
     describe('with 3 people', () => {
       it("assigns roles to pairs that haven't had them for longer", () => {
         const best = Recommendation.calculateMovesToBestAssignment({
@@ -895,11 +946,11 @@ describe('Recommendation', () => {
           expect.arrayContaining([
             {
               lane: 'l1',
-              entities: ['r1'],
+              entities: ['r2'],
             },
             {
               lane: 'l2',
-              entities: ['r2'],
+              entities: ['r1'],
             },
           ])
         );
@@ -994,7 +1045,7 @@ describe('Recommendation', () => {
       });
     });
 
-    describe('less right than lanes', () => {
+    describe('more rights than lanes', () => {
       it('puts multiple on the same lane', () => {
         const best = Recommendation.calculateMovesToBestAssignment({
           left: 'person',
@@ -1062,7 +1113,7 @@ describe('Recommendation', () => {
               entities: ['r2'],
             },
             {
-              lane: 'l1',
+              lane: 'l2',
               entities: ['r3'],
             },
           ])

diff --git a/src/lib/recommendation.ts b/src/lib/recommendation.ts
@@ -469,7 +469,7 @@ export const calculateMovesToBestAssignment = ({ left, right, current, history }
   let maxScore = 0;
 
   if (history && history.length > 0) {
-    maxScore = parseInt(_last(history)['.key']);
+    maxScore = parseInt(key(_last(history)));
 
     history = history.map((h) => {
       const groups = _groupBy(
@@ -480,14 +480,12 @@ export const calculateMovesToBestAssignment = ({ left, right, current, history }
         'location'
       );
       const lanes = [];
-      const score = maxScore - parseInt(h['.key']);
+      const score = maxScore - parseInt(key(h));
 
       Object.values(groups).forEach((entities) => {
-        entities = entities.map(key);
-
         lanes.push({
-          left: entities.filter((e) => e.type === left),
-          right: entities.filter((e) => e.type === right),
+          left: entities.filter((e) => e.type === left).map(key),
+          right: entities.filter((e) => e.type === right).map(key),
         });
       });
       return { score, lanes };

diff --git a/yarn.lock b/yarn.lock
@@ -2277,6 +2277,11 @@
   resolved "https://registry.yarnpkg.com/@types/history/-/history-4.7.8.tgz#49348387983075705fe8f4e02fb67f7daaec4934"
   integrity sha512-S78QIYirQcUoo6UJZx9CSP0O2ix9IaeAXwQi26Rhr/+mg7qqPy8TzaxHSUut7eGjL8WmLccT7/MXf304WjqHcA==
 
+"@types/history@^4.7.11":
+  version "4.7.11"
+  resolved "https://registry.yarnpkg.com/@types/history/-/history-4.7.11.tgz#56588b17ae8f50c53983a524fc3cc47437969d64"
+  integrity sha512-qjDJRrmvBMiTx+jyLxvLfJU7UznFuokDv4f3WRuriHKERccVpFU+8XMQUAbDzoiJCsmexxRExQeMwwCdamSKDA==
+
 "@types/html-minifier-terser@^5.0.0":
   version "5.1.1"
   resolved "https://registry.yarnpkg.com/@types/html-minifier-terser/-/html-minifier-terser-5.1.1.tgz#3c9ee980f1a10d6021ae6632ca3e79ca2ec4fb50"