Fix Organisation tags association

[Re #1629] We were misusing the result of the DB query to update an organisation, as we were using the affected rows number as it was the organisation's updated id, that was being passed to the function to associate tags with the organisation. Hence, we have ended-up associating those tags to the organisation with id = `1` (UNEP). Besides, since we no longer have organisations with id `-1` to handle GPML membership process, we don't need at all to update an id of an organisation there, which is quite risky as well. So, now we avoid that and we also make sure we use the entity's domain properties, removing the ones that are not expected to be updated in each case. Finally, we also provide the right `organisation` id coming from the Path params of the endpoint for making the tags associations. In order to perform the mentioned changes easier, we have refactored the update code to be dynamic and not harcode the updated params. For that we have followed a similar approach as for the resource update query, adding DB functions declarations as well.
akvo · Oct 6, 2023 · dc8c5d8 · dc8c5d8
1 parent d5a0967
commit dc8c5d8
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 65 deletions.
diff --git a/backend/src/gpml/db/organisation.clj b/backend/src/gpml/db/organisation.clj
@@ -1,5 +1,27 @@
 (ns gpml.db.organisation
   {:ns-tracker/resource-deps ["organisation.sql"]}
-  (:require [hugsql.core :as hugsql]))
+  (:require [gpml.util :as util]
+            [gpml.util.sql :as sql-util]
+            [hugsql.core :as hugsql]))
 
-(hugsql/def-db-fns "gpml/db/organisation.sql")
+(declare update-organisation
+         organisation-by-id
+         all-public-entities
+         all-public-non-member-entities
+         all-non-members
+         new-organisation
+         all-members
+         organisation-tags
+         geo-coverage-v2
+         get-organisation-files-to-migrate
+         get-organisations)
+
+(hugsql/def-db-fns "gpml/db/organisation.sql" {:quoting :ansi})
+
+(defn organisation->db-organisation
+  "Apply transformations to Organisation entity fields to database specific
+  types."
+  [resource]
+  (-> resource
+      (util/update-if-not-nil :geo_coverage_type sql-util/keyword->pg-enum "geo_coverage_type")
+      (util/update-if-not-nil :review_status sql-util/keyword->pg-enum "review_status")))
diff --git a/backend/src/gpml/db/organisation.sql b/backend/src/gpml/db/organisation.sql
@@ -98,26 +98,16 @@ values (
 --~ (when (contains? params :logo_id) ", :logo_id")
 ) returning id;
 
--- :name update-organisation :! :n
--- :doc Update organisation column
-update organisation set id = :id
---~ (when (contains? params :name) ",name= :name")
---~ (when (contains? params :subnational_area) ",subnational_area= :subnational_area")
---~ (when (contains? params :url) ",url= :url")
---~ (when (contains? params :type) ",type= :type")
---~ (when (contains? params :country) ",country= :country")
---~ (when (contains? params :program) ",program= :program")
---~ (when (contains? params :representative_group_other) ",representative_group_other= :representative_group_other")
---~ (when (contains? params :representative_group_civil_society) ",representative_group_civil_society= :representative_group_civil_society")
---~ (when (contains? params :representative_group_private_sector) ",representative_group_private_sector= :representative_group_private_sector")
---~ (when (contains? params :representative_group_government) ",representative_group_government= :representative_group_government")
---~ (when (contains? params :representative_group_academia_research) ",representative_group_academia_research= :representative_group_academia_research")
---~ (when (contains? params :geo_coverage_type) ",geo_coverage_type= :geo_coverage_type::geo_coverage_type")
---~ (when (contains? params :created_by) ",created_by= :created_by")
---~ (when (contains? params :is_member) ",is_member= :is_member")
---~ (when (contains? params :logo_id) ", logo_id= :logo_id")
---~ (when (contains? params :review_status) ", review_status= :review_status::review_status")
-where id = :id
+-- :name update-organisation :execute :affected
+UPDATE organisation
+SET
+/*~
+(str/join ","
+  (for [[field _] (:updates params)]
+    (str (identifier-param-quote (name field) options)
+      " = :updates." (name field))))
+~*/
+WHERE id = :id;
 
 -- :name geo-coverage-v2 :? :*
 -- :doc Get geo coverage by organisation id

diff --git a/backend/src/gpml/handler/organisation.clj b/backend/src/gpml/handler/organisation.clj
@@ -11,11 +11,13 @@
             [gpml.handler.responses :as r]
             [gpml.service.file :as srv.file]
             [gpml.service.permissions :as srv.permissions]
+            [gpml.util :as util]
             [gpml.util.email :as email]
             [gpml.util.geo :as geo]
             [gpml.util.malli :as util.malli]
             [gpml.util.postgresql :as pg-util]
             [integrant.core :as ig]
+            [malli.util :as mu]
             [ring.util.response :as resp])
   (:import [java.sql SQLException]))
 
@@ -56,6 +58,8 @@
       :resource-id org-id})
     org-id))
 
+;; TODO: This always expects a logo to provided but that doesn't make sense.
+;; We shouldn't do anything when the `logo` key is not provided.
 (defn- handle-logo-update
   [config conn org-id logo-payload]
   (let [old-org (db.organisation/organisation-by-id conn {:id org-id})
@@ -84,7 +88,18 @@
   (let [org-id (:id org)
         new-logo-id (handle-logo-update config conn org-id logo)
         geo-coverage-type (keyword geo_coverage_type)
-        org-id (db.organisation/update-organisation conn (assoc org :logo_id new-logo-id))]
+        updates-map (-> org
+                        (assoc :logo_id new-logo-id)
+                        (dissoc :id :tags :geo_coverage_countries :geo_coverage_country_groups :logo)
+                        (util/update-if-not-nil :review_status keyword)
+                        (util/update-if-not-nil :geo_coverage_type keyword)
+                        db.organisation/organisation->db-organisation)
+        affected-rows (if-not (empty? updates-map)
+                        (db.organisation/update-organisation
+                         conn
+                         {:id org-id
+                          :updates updates-map})
+                        1)]
     (handler.geo/update-resource-geo-coverage conn
                                               :organisation
                                               org-id
@@ -97,7 +112,8 @@
                                                                              :tag-category "general"
                                                                              :resource-name "organisation"
                                                                              :resource-id org-id}))
-    org-id))
+    {:success? (= 1 affected-rows)
+     :affected-entities affected-rows}))
 
 (defmethod ig/init-key :gpml.handler.organisation/get [_ {:keys [db]}]
   (fn [_]
@@ -188,7 +204,7 @@
 
 (defmethod ig/init-key :gpml.handler.organisation/put
   [_ {:keys [db logger] :as config}]
-  (fn [{:keys [body-params referrer parameters user]}]
+  (fn [{:keys [body-params parameters user]}]
     (try
       (if (h.r.permission/operation-allowed?
            config
@@ -197,10 +213,13 @@
             :entity-id (:id (:path parameters))
             :operation-type :update})
         (jdbc/with-db-transaction [tx (:spec db)]
-          (let [org-id (update-org config tx (-> body-params
-                                                 (assoc :id (:id (:path parameters)))
-                                                 (dissoc :review_status)))]
-            (resp/created referrer (assoc body-params :id org-id))))
+          (let [org-data (-> body-params
+                             (select-keys (util.malli/keys dom.organisation/Organisation))
+                             (dissoc :created :modified :review_status :logo_id :second_contact
+                                     :is_member :reviewed_at :reviewed_by :contribution))
+                {:keys [success?] :as result} (update-org config tx (assoc org-data :id (:id (:path parameters))))]
+            (if success? (r/ok {})
+                (r/server-error result))))
         (r/forbidden {:message "Unauthorized"}))
       (catch Throwable t
         (let [log-data {:exception-message (ex-message t)
@@ -212,32 +231,12 @@
           (r/server-error {:success? false
                            :reason :exception
                            :error-details log-data}))))))
-
-;; TODO: We are not skipping extra params, as for example we don't want `is_member` to be updatable
-;; from this endpoint, so we should skip the ones not expected.
 (defmethod ig/init-key :gpml.handler.organisation/put-params [_ _]
-  (into [:map
-         [:name {:optional true} string?]
-         [:url {:optional true} string?]
-         [:logo {:optional true} string?]
-         [:country {:optional true} int?]
-         [:geo_coverage_type {:optional true} geo/coverage_type]
-         [:type {:optional true} string?]
-         [:representative_group_other {:optional true} [:maybe string?]]
-         [:representative_group_civil_society {:optional true} [:maybe string?]]
-         [:representative_group_private_sector {:optional true} [:maybe string?]]
-         [:representative_group_government {:optional true} [:maybe string?]]
-         [:representative_group_academia_research {:optional true} [:maybe string?]]
-         [:subnational_area {:optional true} [:maybe string?]]
-         [:expertise {:optional true} vector?]
-         [:program {:optional true} string?]
-         [:tags {:optional true}
-          [:vector {:optional true}
-           [:map {:optional true}
-            [:id {:optional true} pos-int?]
-            [:tag string?]
-            [:tag_category {:optional true} string?]]]]]
-        handler.geo/api-geo-coverage-schemas))
+  (-> dom.organisation/Organisation
+      (util.malli/dissoc [:id :created :modified :review_status
+                          :created_by :created_at :second_contact
+                          :is_member :reviewed_at :reviewed_by :contribution])
+      mu/optional-keys))
 
 (defmethod ig/init-key :gpml.handler.organisation/put-req-member
   [_ {:keys [db logger mailjet-config] :as config}]
@@ -258,11 +257,11 @@
                        (= "REJECTED" review_status) {:success? false
                                                      :reason :entity-rejected}
                        :else (jdbc/with-db-transaction [tx (:spec db)]
-                               (update-org config tx (assoc body-params
-                                                            :id org-id
-                                                            :is_member true
-                                                            :review_status "SUBMITTED"))
-                               {:success? true}))]
+                               (update-org config tx (-> body-params
+                                                         (select-keys (util.malli/keys dom.organisation/Organisation))
+                                                         (dissoc :created :modified :logo_id :second_contact
+                                                                 :reviewed_at :reviewed_by)
+                                                         (assoc :id org-id :is_member true :review_status "SUBMITTED")))))]
           (if (:success? result)
             (do
               (email/notify-admins-pending-approval

diff --git a/backend/src/gpml/handler/programmatic/files_migrator.clj b/backend/src/gpml/handler/programmatic/files_migrator.clj
@@ -97,7 +97,8 @@
   (migrate-files config
                  {:get-files-to-migrate-fn db.organisation/get-organisation-files-to-migrate
                   :update-entity-fn (fn [conn id updates] (db.organisation/update-organisation conn
-                                                                                               (merge {:id id} updates)))
+                                                                                               {:id id
+                                                                                                :updates updates}))
                   :entity-key entity-key
                   :visibility :private
                   :limit (:limit opts)}))

diff --git a/backend/src/gpml/service/stakeholder.clj b/backend/src/gpml/service/stakeholder.clj
@@ -139,11 +139,11 @@
                              :success? false
                              :reason :failed-to-assign-sth-owner-role-to-organisation
                              :error-details {:result result})
-                      (let [success? (boolean (handler.org/update-org
-                                               config
-                                               conn
-                                               {:id (:id org)
-                                                :created_by sth-id}))]
+                      (let [{:keys [success?]} (handler.org/update-org
+                                                config
+                                                conn
+                                                {:id (:id org)
+                                                 :created_by sth-id})]
                         (if success?
                           context
                           (assoc context