Merge pull request #7511 from penpot/marina-improve-users-give-feedback

✨ Improve the way users give us feedback
2025-12-11 22:14:05 +01:00 · 2025-10-17 10:19:34 +02:00
parent 3665bccaed fbbee98c3d
commit f5761066a9
18 changed files with 449 additions and 193 deletions
--- a/backend/resources/app/email/feedback/en.html
+++ b/backend/resources/app/email/feedback/en.html
@@ -8,38 +8,41 @@
  <body>
    <p>
      <strong>Feedback from:</strong><br />
-      {% if profile %}
-        <span>
-          <span>Name: </span>
-          <span><code>{{profile.fullname|abbreviate:25}}</code></span>
-        </span>
-        <br />
-
-        <span>
-          <span>Email: </span>
-          <span>{{profile.email}}</span>
-        </span>
-        <br />
-
-        <span>
-          <span>ID: </span>
-          <span><code>{{profile.id}}</code></span>
-        </span>
-      {% else %}
-        <span>
-          <span>Email: </span>
-          <span>{{profile.email}}</span>
-        </span>
-      {% endif %}
+      <span>
+        <span>Name: </span>
+        <span><code>{{profile.fullname|abbreviate:25}}</code></span>
+      </span>
+      <br />
+      <span>
+        <span>Email: </span>
+        <span>{{profile.email}}</span>
+      </span>
+      <br />
+      <span>
+        <span>ID: </span>
+        <span><code>{{profile.id}}</code></span>
+      </span>
    </p>
    <p>
      <strong>Subject:</strong><br />
-      <span>{{subject|abbreviate:300}}</span>
+      <span>{{feedback-subject|abbreviate:300}}</span>
    </p>

+    <p>
+      <strong>Type:</strong><br />
+      <span>{{feedback-type|abbreviate:300}}</span>
+    </p>
+
+    {% if feedback-error-href %}
+    <p>
+      <strong>Error HREF:</strong><br />
+      <span>{{feedback-error-href|abbreviate:500}}</span>
+    </p>
+    {% endif %}
+
    <p>
      <strong>Message:</strong><br />
-      {{content|linebreaks-br|safe}}
+      {{feedback-content|linebreaks-br}}
    </p>
  </body>
 </html>
--- a/backend/resources/app/email/feedback/en.subj
+++ b/backend/resources/app/email/feedback/en.subj
@@ -1 +1 @@
-[PENPOT FEEDBACK]: {{subject}}
+[PENPOT FEEDBACK]: {{feedback-subject}}
--- a/backend/resources/app/email/feedback/en.txt
+++ b/backend/resources/app/email/feedback/en.txt
@@ -1,9 +1,10 @@
-{% if profile %}
-Feedback profile: {{profile.fullname}} <{{profile.email}}> / {{profile.id}}
-{% else %}
-Feedback from: {{email}}
-{% endif %}
+From: {{profile.fullname}} <{{profile.email}}> / {{profile.id}}
+Subject: {{feedback-subject}}
+Type: {{feedback-type}}
+{%- if feedback-error-href %}
+HREF: {{feedback-error-href}}
+{% endif -%}

-Subject: {{subject}}
+Message:

-{{content}}
+{{feedback-content}}
--- a/backend/scripts/_env
+++ b/backend/scripts/_env
@@ -20,6 +20,7 @@ export PENPOT_FLAGS="\
       enable-audit-log \
       enable-transit-readable-response \
       enable-demo-users \
+       enable-user-feedback \
       disable-secure-session-cookies \
       enable-smtp \
       enable-prepl-server \
@@ -46,6 +47,8 @@ export PENPOT_MEDIA_MAX_FILE_SIZE=104857600
 # Setup default multipart upload size to 300MiB
 export PENPOT_HTTP_SERVER_MAX_MULTIPART_BODY_SIZE=314572800

+export PENPOT_USER_FEEDBACK_DESTINATION="support@example.com"
+
 export AWS_ACCESS_KEY_ID=penpot-devenv
 export AWS_SECRET_ACCESS_KEY=penpot-devenv
 export PENPOT_OBJECTS_STORAGE_BACKEND=s3
--- a/backend/src/app/email.clj
+++ b/backend/src/app/email.clj
@@ -7,6 +7,7 @@
 (ns app.email
  "Main api for send emails."
  (:require
+   [app.common.data :as d]
   [app.common.data.macros :as dm]
   [app.common.exceptions :as ex]
   [app.common.logging :as l]
@@ -93,36 +94,44 @@
               headers)))

 (defn- assign-body
-  [^MimeMessage mmsg {:keys [body charset] :or {charset "utf-8"}}]
-  (let [mpart (MimeMultipart. "mixed")]
+  [^MimeMessage mmsg {:keys [body charset attachments] :or {charset "utf-8"}}]
+  (let [mixed-mpart  (MimeMultipart. "mixed")]
    (cond
      (string? body)
-      (let [bpart (MimeBodyPart.)]
-        (.setContent bpart ^String body (str "text/plain; charset=" charset))
-        (.addBodyPart mpart bpart))
-
-      (vector? body)
-      (let [mmp (MimeMultipart. "alternative")
-            mbp (MimeBodyPart.)]
-        (.addBodyPart mpart mbp)
-        (.setContent mbp mmp)
-        (doseq [item body]
-          (let [mbp (MimeBodyPart.)]
-            (.setContent mbp
-                         ^String (:content item)
-                         ^String (str (:type item "text/plain") "; charset=" charset))
-            (.addBodyPart mmp mbp))))
+      (let [text-part (MimeBodyPart.)]
+        (.setText text-part ^String body ^String charset)
+        (.addBodyPart mixed-mpart text-part))

      (map? body)
-      (let [bpart (MimeBodyPart.)]
-        (.setContent bpart
-                     ^String (:content body)
-                     ^String (str (:type body "text/plain") "; charset=" charset))
-        (.addBodyPart mpart bpart))
+      (let [content-part      (MimeBodyPart.)
+            alternative-mpart (MimeMultipart. "alternative")]
+
+        (when-let [content (get body "text/html")]
+          (let [html-part (MimeBodyPart.)]
+            (.setContent html-part ^String content
+                         (str "text/html; charset=" charset))
+            (.addBodyPart alternative-mpart html-part)))
+
+        (when-let [content (get body "text/plain")]
+          (let [text-part (MimeBodyPart.)]
+            (.setText text-part ^String content ^String charset)
+            (.addBodyPart alternative-mpart text-part)))
+
+        (.setContent content-part alternative-mpart)
+        (.addBodyPart mixed-mpart content-part))

      :else
-      (throw (ex-info "Unsupported type" {:body body})))
-    (.setContent mmsg mpart)
+      (throw (IllegalArgumentException. "invalid email body provided")))
+
+    (doseq [[name content] attachments]
+
+      (prn "attachment" name)
+      (let [attachment-part (MimeBodyPart.)]
+        (.setFileName attachment-part ^String name)
+        (.setContent attachment-part ^String content (str "text/plain; charset=" charset))
+        (.addBodyPart mixed-mpart attachment-part)))
+
+    (.setContent mmsg mixed-mpart)
    mmsg))

 (defn- opts->props
@@ -210,24 +219,26 @@
      (ex/raise :type :internal
                :code :missing-email-templates))
    {:subject subj
-     :body (into
-            [{:type "text/plain"
-              :content text}]
-            (when html
-              [{:type "text/html"
-                :content html}]))}))
+     :body (d/without-nils
+            {"text/plain" text
+             "text/html" html})}))

-(def ^:private schema:context
-  [:map
+(def ^:private schema:params
+  [:map {:title "Email Params"}
   [:to [:or ::sm/email [::sm/vec ::sm/email]]]
   [:reply-to {:optional true} ::sm/email]
   [:from {:optional true} ::sm/email]
   [:lang {:optional true} ::sm/text]
+   [:subject {:optional true} ::sm/text]
   [:priority {:optional true} [:enum :high :low]]
-   [:extra-data {:optional true} ::sm/text]])
+   [:extra-data {:optional true} ::sm/text]
+   [:body {:optional true}
+    [:or :string [:map-of :string :string]]]
+   [:attachments {:optional true}
+    [:map-of :string :string]]])

-(def ^:private check-context
-  (sm/check-fn schema:context))
+(def ^:private check-params
+  (sm/check-fn schema:params))

 (defn template-factory
  [& {:keys [id schema]}]
@@ -235,9 +246,9 @@
  (let [check-fn (if schema
                   (sm/check-fn schema)
                   (constantly nil))]
-    (fn [context]
-      (let [context (-> context check-context check-fn)
-            email   (build-email-template id context)]
+    (fn [params]
+      (let [params (-> params check-params check-fn)
+            email  (build-email-template id params)]
        (when-not email
          (ex/raise :type :internal
                    :code :email-template-does-not-exists
@@ -245,35 +256,40 @@
                    :template-id id))

        (cond-> (assoc email :id (name id))
-          (:extra-data context)
-          (assoc :extra-data (:extra-data context))
+          (:extra-data params)
+          (assoc :extra-data (:extra-data params))

-          (:from context)
-          (assoc :from (:from context))
+          (seq (:attachments params))
+          (assoc :attachments (:attachments params))

-          (:reply-to context)
-          (assoc :reply-to (:reply-to context))
+          (:from params)
+          (assoc :from (:from params))

-          (:to context)
-          (assoc :to (:to context)))))))
+          (:reply-to params)
+          (assoc :reply-to (:reply-to params))
+
+          (:to params)
+          (assoc :to (:to params)))))))

 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; PUBLIC HIGH-LEVEL API
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

 (defn render
-  [email-factory context]
-  (email-factory context))
+  [email-factory params]
+  (email-factory params))

 (defn send!
  "Schedule an already defined email to be sent using asynchronously
  using worker task."
-  [{:keys [::conn ::factory] :as context}]
+  [{:keys [::conn ::factory] :as params}]
  (assert (db/connectable? conn) "expected a valid database connection or pool")

  (let [email (if factory
-                (factory context)
-                (dissoc context ::conn))]
+                (factory params)
+                (-> params
+                    (dissoc params)
+                    (check-params)))]
    (wrk/submit! {::wrk/task :sendmail
                  ::wrk/delay 0
                  ::wrk/max-retries 4
@@ -343,8 +359,10 @@

 (def ^:private schema:feedback
  [:map
-   [:subject ::sm/text]
-   [:content ::sm/text]])
+   [:feedback-subject ::sm/text]
+   [:feedback-type ::sm/text]
+   [:feedback-content ::sm/text]
+   [:profile :map]])

 (def user-feedback
  "A profile feedback email."
--- a/backend/src/app/rpc/commands/feedback.clj
+++ b/backend/src/app/rpc/commands/feedback.clj
@@ -7,6 +7,7 @@
 (ns app.rpc.commands.feedback
  "A general purpose feedback module."
  (:require
+   [app.common.data :as d]
   [app.common.exceptions :as ex]
   [app.common.schema :as sm]
   [app.config :as cf]
@@ -21,8 +22,11 @@

 (def ^:private schema:send-user-feedback
  [:map {:title "send-user-feedback"}
-   [:subject [:string {:max 400}]]
-   [:content [:string {:max 2500}]]])
+   [:subject [:string {:max 500}]]
+   [:content [:string {:max 2500}]]
+   [:type {:optional true} :string]
+   [:error-href {:optional true} [:string {:max 2500}]]
+   [:error-report {:optional true} :string]])

 (sv/defmethod ::send-user-feedback
  {::doc/added "1.18"
@@ -39,16 +43,26 @@

 (defn- send-user-feedback!
  [pool profile params]
-  (let [dest (or (cf/get :user-feedback-destination)
-                 ;; LEGACY
-                 (cf/get :feedback-destination))]
+  (let [destination
+        (or (cf/get :user-feedback-destination)
+            ;; LEGACY
+            (cf/get :feedback-destination))
+
+        attachments
+        (d/without-nils
+         {"error-report.txt" (:error-report params)})]
+
    (eml/send! {::eml/conn pool
                ::eml/factory eml/user-feedback
-                :from     dest
-                :to       dest
-                :profile  profile
+                :from     (cf/get :smtp-default-from)
+                :to       destination
                :reply-to (:email profile)
                :email    (:email profile)
-                :subject  (:subject params)
-                :content  (:content params)})
+                :attachments attachments
+
+                :feedback-subject (:subject params)
+                :feedback-type (:type params "not-specified")
+                :feedback-content (:content params)
+                :feedback-error-href (:error-href params)
+                :profile profile})
    nil))
--- a/backend/test/backend_tests/email_sending_test.clj
+++ b/backend/test/backend_tests/email_sending_test.clj
@@ -22,4 +22,4 @@
    (t/is (contains? result :body))
    (t/is (contains? result :to))
    #_(t/is (contains? result :reply-to))
-    (t/is (vector? (:body result)))))
+    (t/is (map? (:body result)))))