Merge pull request #3815 from aerodeval/fix/user-email-permlevel-reset

fix(profile): save agent email links through the helpdesk api

desk/src/components/Settings/Profile/UserEmailSettings.vue

@@ -10,7 +10,7 @@
           <Button
             variant="solid"
             :label="__('Update')"
-            :loading="user?.save?.loading"
+            :loading="user.save.loading || saveEmailAccountsResource.loading"
             @click="update"
           /></div
       ></Transition>
@@ -51,7 +51,7 @@
         </div>
         <div>
           <div
-            v-if="user.doc.user_emails?.length"
+            v-if="linkedEmails.length"
             class="w-full border rounded-5 mb-2 border-outline-elevation-2"
           >
             <div
@@ -62,8 +62,8 @@
               <span></span>
             </div>
             <div
-              v-for="e in user.doc.user_emails"
-              :key="e.name"
+              v-for="e in linkedEmails"
+              :key="e.email_account"
               class="grid grid-cols-[4fr_4fr_0.3fr] gap-2 group items-center px-4 py-2.5 text-base border-b border-outline-elevation-2 last:border-b-0"
             >
               <span class="text-ink-gray-8 font-medium truncate">
@@ -134,7 +134,13 @@ import { getUserEmailInfo } from "@/composables/useUserEmailInfo";
 import { useAuthStore } from "@/stores/auth";
 import { __ } from "@/translation";
 import { normalize, uploadFunction } from "@/utils";
-import { Button, Combobox, createDocumentResource, toast } from "frappe-ui";
+import {
+  Button,
+  Combobox,
+  createDocumentResource,
+  createResource,
+  toast,
+} from "frappe-ui";
 import { computed, ref, watch } from "vue";
 import { disableSettingModalOutsideClick } from "../settingsModal";
 
@@ -144,16 +150,41 @@ const emit = defineEmits(["updateStep"]);
 
 const currentUserEmailInfo = getUserEmailInfo();
 
+// user_emails is System Manager-only, so the User doc never carries it for
+// agents; the list comes from the email info API and is saved through it
+const linkedEmails = ref([]);
+watch(
+  () => currentUserEmailInfo.data?.outgoing_emails,
+  (emails, previousEmails) => {
+    // the info is shared with the composer; a reload from there must not
+    // wipe unsaved edits here
+    if (
+      sortedAccountNames(linkedEmails.value) !==
+      sortedAccountNames(previousEmails)
+    )
+      return;
+    linkedEmails.value = [...(emails || [])];
+  },
+  { immediate: true }
+);
+
+const saveEmailAccountsResource = createResource({
+  url: "helpdesk.api.auth.set_user_email_accounts",
+  makeParams: () => ({
+    email_accounts: linkedAccountNames(linkedEmails.value),
+  }),
+});
+
 const filteredEmails = computed(() => {
   if (!currentUserEmailInfo.data?.available_emails) return [];
-  const linkedEmails = user.doc.user_emails?.map((e) => e.email_id) || [];
+  const linkedAccounts = linkedAccountNames(linkedEmails.value);
   return currentUserEmailInfo.data.available_emails
     .map((doc) => ({
       label: doc.name,
       value: doc.name,
       email: doc.email_id,
     }))
-    .filter((e) => !linkedEmails.includes(e.email));
+    .filter((e) => !linkedAccounts.includes(e.value));
 });
 
 const isSignatureDirty = computed(() => {
@@ -164,10 +195,9 @@ const isSignatureDirty = computed(() => {
 });
 
 const isUserEmailListDirty = computed(() => {
-  const emailIds = (list = []) => list.map((e) => e.email_id).sort();
   return (
-    JSON.stringify(emailIds(currentUserEmailInfo.data?.outgoing_emails)) !==
-    JSON.stringify(emailIds(user.doc.user_emails))
+    sortedAccountNames(currentUserEmailInfo.data?.outgoing_emails) !==
+    sortedAccountNames(linkedEmails.value)
   );
 });
 
@@ -181,28 +211,39 @@ if (isDirty.value) {
   disableSettingModalOutsideClick.value = false;
 }
 
+function linkedAccountNames(list = []) {
+  return list.map((e) => e.email_account);
+}
+
+function sortedAccountNames(list) {
+  return linkedAccountNames(list).sort().join();
+}
+
 function addEmail(email) {
-  if (!user.doc.user_emails) user.doc.user_emails = [];
-  user.doc.user_emails.push({
-    email_account: email.label,
+  linkedEmails.value.push({
+    email_account: email.value,
     email_id: email.email,
   });
 }
 
 function removeEmail(email) {
-  user.doc.user_emails = user.doc.user_emails.filter(
-    (e) => e.email_id !== email.email_id
+  linkedEmails.value = linkedEmails.value.filter(
+    (e) => e.email_account !== email.email_account
   );
 }
 
-function update() {
-  user.save.submit(null, {
-    onSuccess: () => {
-      toast.success(__("Email settings updated successfully."));
-      currentUserEmailInfo.reload();
-      user.reload();
-    },
-  });
+async function update() {
+  // one after the other, since both save the same User doc
+  try {
+    if (isSignatureDirty.value) await user.save.submit();
+    if (isUserEmailListDirty.value) await saveEmailAccountsResource.submit();
+  } catch {
+    return; // the fallback error handler already showed a toast
+  }
+  toast.success(__("Email settings updated successfully."));
+  user.reload();
+  await currentUserEmailInfo.reload();
+  linkedEmails.value = [...currentUserEmailInfo.data.outgoing_emails];
 }
 
 watch(isDirty, (val) => {

helpdesk/api/auth.py

@@ -91,25 +91,53 @@ def get_current_user_email_info():
         filters={"parent": user},
         fields=["email_account", "email_id"],
     )
-    outgoing_account_names = frappe.db.get_all(
-        "Email Account",
-        filters={"enable_outgoing": 1},
-        pluck="name",
-    )
-
-    outgoing_emails = [
-        row for row in user_emails if row.email_account in outgoing_account_names
-    ]
-
     available_emails = frappe.db.get_all(
         "Email Account",
         filters={"enable_outgoing": 1},
         fields=["name", "email_id"],
     )
 
+    outgoing_account_names = {account.name for account in available_emails}
+    outgoing_emails = [
+        row for row in user_emails if row.email_account in outgoing_account_names
+    ]
+
     return {
         "email_signature": email_signature,
         "email": email,
         "outgoing_emails": outgoing_emails,
         "available_emails": available_emails,
     }
+
+
+@frappe.whitelist(methods=["POST"])
+@agent_only
+def set_user_email_accounts(email_accounts: list[str]):
+    """Link the given outgoing email accounts to the current user.
+
+    User Emails is a System Manager-only table, so an agent can neither read
+    nor save it through their User document. Links to accounts without
+    outgoing email are kept.
+    """
+    outgoing_accounts = dict(
+        frappe.get_all(
+            "Email Account",
+            filters={"enable_outgoing": 1},
+            fields=["name", "email_id"],
+            as_list=True,
+        )
+    )
+    user = frappe.get_doc("User", frappe.session.user)
+    rows = {row.email_account: row for row in user.user_emails}
+    kept_links = [
+        row for row in user.user_emails if row.email_account not in outgoing_accounts
+    ]
+    agent_links = [
+        rows.get(account)
+        or {"email_account": account, "email_id": outgoing_accounts[account]}
+        for account in dict.fromkeys(email_accounts)
+        if account in outgoing_accounts
+    ]
+    user.set("user_emails", kept_links + agent_links)
+    user.flags.ignore_permlevel_for_fields = ["user_emails"]
+    user.save()

helpdesk/test_utils.py

@@ -949,6 +949,23 @@ def make_email_account_data(service: str | None, **overrides: Any) -> dict[str,
     }
 
 
+def make_email_account(service: str, **overrides: Any) -> str:
+    """An Email Account saved through the helpdesk settings API.
+
+    Patch EmailServer first, or it tries to log in to the provider.
+    """
+    from helpdesk.api.settings.email import create_email_account
+
+    return create_email_account(make_email_account_data(service, **overrides))
+
+
+def get_linked_email_accounts(user: str) -> set[str]:
+    """The Email Accounts linked to a user through their User Emails table."""
+    return set(
+        frappe.get_all("User Email", filters={"parent": user}, pluck="email_account")
+    )
+
+
 def get_user_roles(user: str) -> list[str]:
     """A user's Has Role rows, duplicates included (unlike frappe.get_roles)."""
     return frappe.get_all(

helpdesk/tests/api/settings/test_email.py

@@ -5,10 +5,14 @@ from unittest.mock import MagicMock, patch
 import frappe
 from frappe.tests import IntegrationTestCase
 
+from helpdesk.api.auth import get_current_user_email_info, set_user_email_accounts
 from helpdesk.api.settings.email import create_email_account
 from helpdesk.test_utils import (
     create_agent,
+    create_user,
+    get_linked_email_accounts,
     make_agent_manager,
+    make_email_account,
     make_email_account_data,
     unique_email,
 )
@@ -148,3 +152,47 @@ class TestCreateEmailAccount(IntegrationTestCase):
     def assert_ticket_inbox_folder(self, account) -> None:
         folders = [(row.folder_name, row.append_to) for row in account.imap_folder]
         self.assertEqual(folders, [("INBOX", "HD Ticket")])
+
+
+class TestSetUserEmailAccounts(IntegrationTestCase):
+    """Agents manage links to outgoing accounts only; other links stay."""
+
+    def setUp(self) -> None:
+        frappe.set_user("Administrator")
+        self.enterContext(patch(f"{EMAIL_ACCOUNT_MODULE}.EmailServer"))
+        self.agent = create_agent(unique_email("email-agent")).name
+        self.support = make_email_account("Sendgrid", enable_incoming=0)
+        self.sales = make_email_account("Sendgrid", enable_incoming=0)
+        self.incoming_only = make_email_account("GMail", enable_outgoing=0)
+
+    def test_agent_replaces_outgoing_links_and_keeps_other_links(self) -> None:
+        user = frappe.get_doc("User", self.agent)
+        user.append("user_emails", {"email_account": self.incoming_only})
+        user.save()
+
+        with self.set_user(self.agent):
+            set_user_email_accounts([self.support])
+            self.assertEqual(
+                get_linked_email_accounts(self.agent),
+                {self.incoming_only, self.support},
+            )
+            # the settings screen lists only the outgoing links
+            outgoing = get_current_user_email_info()["outgoing_emails"]
+            self.assertEqual([row.email_account for row in outgoing], [self.support])
+
+            set_user_email_accounts([self.sales, self.sales])
+            self.assertEqual(
+                get_linked_email_accounts(self.agent), {self.incoming_only, self.sales}
+            )
+
+    def test_accounts_without_outgoing_email_are_not_linked(self) -> None:
+        with self.set_user(self.agent):
+            set_user_email_accounts([self.incoming_only, "No Such Account"])
+
+        self.assertEqual(get_linked_email_accounts(self.agent), set())
+
+    def test_non_agent_cannot_set_email_accounts(self) -> None:
+        user = create_user(unique_email("not-an-agent")).name
+
+        with self.set_user(user), self.assertRaises(frappe.PermissionError):
+            set_user_email_accounts([self.support])