From 856a009dd8de39b64d57a8d04f139906e9448c08 Mon Sep 17 00:00:00 2001
From: Michael Genson <71845777+michael-genson@users.noreply.github.com>
Date: Sun, 8 Jan 2023 11:23:24 -0600
Subject: [PATCH] fix: for several Shopping List bugs (#1912)

* prevent list refresh while re-ordering items

* update position of new items to stay at the bottom

* prevent refresh while loading

* copy item while editing so it isn't refreshed

* added loading count to handle overlapping actions

* fixed recipe reference throttling

* prevent merging checked and unchecked items
---
 .../Domain/ShoppingList/ShoppingListItem.vue  | 31 +++++--
 frontend/pages/shopping-lists/_id.vue         | 81 ++++++++++---------
 .../services/group_services/shopping_lists.py |  4 +
 3 files changed, 72 insertions(+), 44 deletions(-)

diff --git a/frontend/components/Domain/ShoppingList/ShoppingListItem.vue b/frontend/components/Domain/ShoppingList/ShoppingListItem.vue
index 081cab71..5af1f40d 100644
--- a/frontend/components/Domain/ShoppingList/ShoppingListItem.vue
+++ b/frontend/components/Domain/ShoppingList/ShoppingListItem.vue
@@ -6,7 +6,7 @@
       hide-details
       dense
       :label="listItem.note"
-      @change="$emit('checked')"
+      @change="$emit('checked', listItem)"
     >
       <template #label>
         <div :class="listItem.checked ? 'strike-through' : ''">
@@ -30,7 +30,7 @@
           </v-list-item>
         </v-list>
       </v-menu>
-      <v-btn small class="ml-2 mt-2 handle" icon @click="edit = true">
+      <v-btn small class="ml-2 mt-2 handle" icon @click="toggleEdit(true)">
         <v-icon>
           {{ $globals.icons.edit }}
         </v-icon>
@@ -39,14 +39,14 @@
   </div>
   <div v-else class="mb-1 mt-6">
     <ShoppingListItemEditor
-      v-model="listItem"
+      v-model="localListItem"
       :labels="labels"
       :units="units"
       :foods="foods"
       @save="save"
-      @cancel="edit = !edit"
+      @cancel="toggleEdit(false)"
       @delete="$emit('delete')"
-      @toggle-foods="listItem.isFood = !listItem.isFood"
+      @toggle-foods="localListItem.isFood = !localListItem.isFood"
     />
   </div>
 </template>
@@ -104,24 +104,37 @@ export default defineComponent({
       },
     ];
 
+    // copy prop value so a refresh doesn't interrupt the user
+    const localListItem = ref(Object.assign({}, props.value));
     const listItem = computed({
       get: () => {
         return props.value;
       },
       set: (val) => {
+        // keep local copy in sync
+        localListItem.value = val;
         context.emit("input", val);
       },
     });
     const edit = ref(false);
+    function toggleEdit(val = !edit.value) {
+      if (val) {
+        // update local copy of item with the current value
+        localListItem.value = props.value;
+      }
+
+      edit.value = val;
+    }
+
     function contextHandler(event: string) {
       if (event === "edit") {
-        edit.value = true;
+        toggleEdit(true);
       } else {
         context.emit(event);
       }
     }
     function save() {
-      context.emit("save");
+      context.emit("save", localListItem.value);
       edit.value = false;
     }
 
@@ -139,7 +152,7 @@ export default defineComponent({
     );
 
     /**
-     * Get's the label for the shopping list item. Either the label assign to the item
+     * Gets the label for the shopping list item. Either the label assign to the item
      * or the label of the food applied.
      */
     const label = computed<MultiPurposeLabelSummary | undefined>(() => {
@@ -164,7 +177,9 @@ export default defineComponent({
       edit,
       contextMenu,
       listItem,
+      localListItem,
       label,
+      toggleEdit,
     };
   },
 });
diff --git a/frontend/pages/shopping-lists/_id.vue b/frontend/pages/shopping-lists/_id.vue
index 8ef19911..f6aa940a 100644
--- a/frontend/pages/shopping-lists/_id.vue
+++ b/frontend/pages/shopping-lists/_id.vue
@@ -10,7 +10,7 @@
     <!-- Viewer -->
     <section v-if="!edit" class="py-2">
       <div v-if="!byLabel">
-        <draggable :value="shoppingList.listItems" handle=".handle" @input="updateIndex">
+        <draggable :value="shoppingList.listItems" handle=".handle" @start="loadingCounter += 1" @end="loadingCounter -= 1" @input="updateIndex">
           <v-lazy v-for="(item, index) in listItems.unchecked" :key="item.id">
             <ShoppingListItem
               v-model="listItems.unchecked[index]"
@@ -18,8 +18,8 @@
               :labels="allLabels || []"
               :units="allUnits || []"
               :foods="allFoods || []"
-              @checked="saveListItem(item)"
-              @save="saveListItem(item)"
+              @checked="saveListItem"
+              @save="saveListItem"
               @delete="deleteListItem(item)"
             />
           </v-lazy>
@@ -43,8 +43,8 @@
               :labels="allLabels || []"
               :units="allUnits || []"
               :foods="allFoods || []"
-              @checked="saveListItem(item)"
-              @save="saveListItem(item)"
+              @checked="saveListItem"
+              @save="saveListItem"
               @delete="deleteListItem(item)"
             />
           </v-lazy>
@@ -134,8 +134,8 @@
                 :labels="allLabels"
                 :units="allUnits || []"
                 :foods="allFoods || []"
-                @checked="saveListItem(item)"
-                @save="saveListItem(item)"
+                @checked="saveListItem"
+                @save="saveListItem"
                 @delete="deleteListItem(item)"
               />
             </div>
@@ -215,7 +215,8 @@ export default defineComponent({
   },
   setup() {
     const { idle } = useIdle(5 * 60 * 1000) // 5 minutes
-    const loading = ref(true);
+    const loadingCounter = ref(1);
+    const recipeReferenceLoading = ref(false);
     const userApi = useUserApi();
 
     const edit = ref(false);
@@ -237,13 +238,20 @@ export default defineComponent({
     }
 
     async function refresh() {
-      shoppingList.value = await fetchShoppingList();
+      loadingCounter.value += 1;
+      const newListValue = await fetchShoppingList();
+      loadingCounter.value -= 1;
+
+      // only update the list with the new value if we're not loading, to prevent UI jitter
+      if (!loadingCounter.value) {
+        shoppingList.value = newListValue;
+      }
     }
 
     // constantly polls for changes
     async function pollForChanges() {
       // pause polling if the user isn't active or we're busy
-      if (idle.value || loading.value) {
+      if (idle.value || loadingCounter.value) {
         return;
       }
 
@@ -270,7 +278,7 @@ export default defineComponent({
     }
 
     // start polling
-    loading.value = false;
+    loadingCounter.value -= 1;
     const pollFrequency = 5000;
 
     let attempts = 0;
@@ -340,11 +348,11 @@ export default defineComponent({
         return;
       }
 
-      loading.value = true;
+      loadingCounter.value += 1;
       deleteListItems(checked);
 
+      loadingCounter.value -= 1;
       refresh();
-      loading.value = false;
     }
 
     // =====================================
@@ -458,33 +466,35 @@ export default defineComponent({
     });
 
     async function addRecipeReferenceToList(recipeId: string) {
-      if (!shoppingList.value || loading.value) {
+      if (!shoppingList.value || recipeReferenceLoading.value) {
         return;
       }
 
-      loading.value = true;
+      loadingCounter.value += 1;
+      recipeReferenceLoading.value = true;
       const { data } = await userApi.shopping.lists.addRecipe(shoppingList.value.id, recipeId);
+      recipeReferenceLoading.value = false;
+      loadingCounter.value -= 1;
 
       if (data) {
         refresh();
       }
-
-      loading.value = false;
     }
 
     async function removeRecipeReferenceToList(recipeId: string) {
-      if (!shoppingList.value || loading.value) {
+      if (!shoppingList.value || recipeReferenceLoading.value) {
         return;
       }
 
-      loading.value = true;
+      loadingCounter.value += 1;
+      recipeReferenceLoading.value = true;
       const { data } = await userApi.shopping.lists.removeRecipe(shoppingList.value.id, recipeId);
+      recipeReferenceLoading.value = false;
+      loadingCounter.value -= 1;
 
       if (data) {
         refresh();
       }
-
-      loading.value = false;
     }
 
     // =====================================
@@ -500,7 +510,7 @@ export default defineComponent({
         return;
       }
 
-      loading.value = true;
+      loadingCounter.value += 1;
       if (item.checked && shoppingList.value.listItems) {
         const lst = shoppingList.value.listItems.filter((itm) => itm.id !== item.id);
         lst.push(item);
@@ -508,12 +518,11 @@ export default defineComponent({
       }
 
       const { data } = await userApi.shopping.items.updateOne(item.id, item);
+      loadingCounter.value -= 1;
 
       if (data) {
         refresh();
       }
-
-      loading.value = false;
     }
 
     async function deleteListItem(item: ShoppingListItemOut) {
@@ -521,14 +530,13 @@ export default defineComponent({
         return;
       }
 
-      loading.value = true;
+      loadingCounter.value += 1;
       const { data } = await userApi.shopping.items.deleteOne(item.id);
+      loadingCounter.value -= 1;
 
       if (data) {
         refresh();
       }
-
-      loading.value = false;
     }
 
     // =====================================
@@ -556,16 +564,18 @@ export default defineComponent({
         return;
       }
 
-      loading.value = true;
+      loadingCounter.value += 1;
+
+      // make sure it's inserted into the end of the list, which may have been updated
+      createListItemData.value.position = shoppingList.value?.listItems?.length || 1;
       const { data } = await userApi.shopping.items.createOne(createListItemData.value);
+      loadingCounter.value -= 1;
 
       if (data) {
         createListItemData.value = ingredientResetFactory();
         createEditorOpen.value = false;
         refresh();
       }
-
-      loading.value = false;
     }
 
     function updateIndex(data: ShoppingListItemOut[]) {
@@ -581,14 +591,13 @@ export default defineComponent({
         return;
       }
 
-      loading.value = true;
+      loadingCounter.value += 1;
       const { data } = await userApi.shopping.items.deleteMany(items);
+      loadingCounter.value -= 1;
 
       if (data) {
         refresh();
       }
-
-      loading.value = false;
     }
 
     async function updateListItems() {
@@ -602,14 +611,13 @@ export default defineComponent({
         return itm;
       });
 
-      loading.value = true;
+      loadingCounter.value += 1;
       const { data } = await userApi.shopping.items.updateMany(shoppingList.value.listItems);
+      loadingCounter.value -= 1;
 
       if (data) {
         refresh();
       }
-
-      loading.value = false;
     }
 
     return {
@@ -629,6 +637,7 @@ export default defineComponent({
       itemsByLabel,
       listItems,
       listRecipes,
+      loadingCounter,
       presentLabels,
       removeRecipeReferenceToList,
       saveListItem,
diff --git a/mealie/services/group_services/shopping_lists.py b/mealie/services/group_services/shopping_lists.py
index ae21066b..343792f7 100644
--- a/mealie/services/group_services/shopping_lists.py
+++ b/mealie/services/group_services/shopping_lists.py
@@ -28,6 +28,10 @@ class ShoppingListService:
         can_merge checks if the two items can be merged together.
         """
 
+        # Check if items are both checked or both unchecked
+        if item1.checked != item2.checked:
+            return False
+
         # Check if foods are equal
         foods_is_none = item1.food_id is None and item2.food_id is None
         foods_not_none = not foods_is_none