Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workflow licenses #2973

Merged
merged 25 commits into from
Sep 9, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
4c64343
feat: migrate workflow_licenses to workflow json blob
aatkin Jul 14, 2022
1c20d51
feat: remove workflow_licenses table migration
aatkin Jul 14, 2022
1fe38eb
feat: enrich application model with workflow licenses
aatkin Jul 14, 2022
7907712
feat: UI to create/edit workflow licenses
aatkin Jul 14, 2022
d6577e6
refactor: minor renamings
aatkin Jul 14, 2022
0074023
refactor: revert and reorganize
aatkin Jul 15, 2022
01416d9
refactor: revert and reorganize part 2
aatkin Jul 15, 2022
b681b20
refactor: render read-only wf licenses like forms
aatkin Jul 18, 2022
2552fb6
chore: fix browser test
aatkin Jul 18, 2022
71df995
chore: fix browser test part 2
aatkin Jul 18, 2022
262551e
doc: update CHANGELOG
aatkin Jul 19, 2022
cabcd36
Merge remote-tracking branch 'origin/master' into 2158/workflow-licenses
aatkin Sep 6, 2022
4412120
feat: disable editing workflow licenses
aatkin Sep 6, 2022
f999d23
refactor: remove licenses from edit workflow, tweak licenses rendering
aatkin Sep 9, 2022
1134926
refactor: add if not exists to workflow_licenses down migration
aatkin Sep 9, 2022
fc8a298
refactor: update fe tests
aatkin Sep 9, 2022
b053b9c
refactor: remove rems.common.util/distinct-by in favor of medley, ref…
aatkin Sep 9, 2022
a5f2cd2
refactor: add missing distinct-by import
aatkin Sep 9, 2022
3b1e514
refactor: browser-test edit workflow cannot edit licenses
aatkin Sep 9, 2022
e240ffa
refactor: remove license change from edit workflow tests
aatkin Sep 9, 2022
52b5296
refactor: update edit-workflow test
aatkin Sep 9, 2022
148f3c8
refactor: fix typo in test
aatkin Sep 9, 2022
91e041b
Merge remote-tracking branch 'origin/master' into 2158/workflow-licenses
aatkin Sep 9, 2022
9f64e04
Merge remote-tracking branch 'origin/master' into 2158/workflow-licenses
aatkin Sep 9, 2022
c7a46ac
chore: fix edit workflow test
aatkin Sep 9, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{:ns rems.migrations.refactor-workflow-licenses
:up-fn migrate-up
:down-fn nil}
22 changes: 1 addition & 21 deletions resources/sql/queries.sql
Original file line number Diff line number Diff line change
Expand Up @@ -405,24 +405,13 @@ SET
id = id
WHERE id = :id;

-- :name create-workflow-license! :insert
INSERT INTO workflow_licenses
(wfid, licid)
VALUES
(:wfid, :licid);

-- TODO: consider renaming this to link-resource-license!
-- :name create-resource-license! :insert
INSERT INTO resource_licenses
(resid, licid)
VALUES
(:resid, :licid);

-- :name get-workflow-licenses :? :*
SELECT licid
FROM workflow_licenses
WHERE wfid = :wfid

-- :name get-workflow :? :1
SELECT
wf.id, wf.organization, wf.title,
Expand All @@ -448,16 +437,10 @@ FROM workflow wf;

-- :name get-licenses :? :*
-- :doc
-- - Gets application licenses by workflow and catalogue item ids
-- - :wfid workflow id for workflow licenses
-- - Gets application licenses by catalogue item ids
-- - :items vector of catalogue item ids for resource licenses
SELECT lic.id, lic.type, lic.enabled, lic.archived, lic.organization
FROM license lic
INNER JOIN workflow_licenses wl ON lic.id = wl.licid
WHERE wl.wfid = :wfid
UNION
SELECT lic.id, lic.type, lic.enabled, lic.archived, lic.organization
FROM license lic
INNER JOIN resource_licenses rl ON lic.id = rl.licid
INNER JOIN catalogue_item item ON (item.resid = rl.resid)
WHERE item.id IN (:v*:items)
Expand Down Expand Up @@ -485,9 +468,6 @@ FROM license_localization;
-- :name get-resources-for-license :? :*
SELECT resid FROM resource_licenses WHERE licid = :id;

-- :name get-workflows-for-license :? :*
SELECT wfid FROM workflow_licenses WHERE licid = :id;

-- :name get-all-roles :? :*
SELECT userid, role
FROM roles;
Expand Down
20 changes: 10 additions & 10 deletions src/clj/rems/api/schema.clj
Original file line number Diff line number Diff line change
Expand Up @@ -107,22 +107,22 @@
(s/optional-key :resource/duo) {:duo/codes [schema-base/DuoCodeFull]}})

(s/defschema V2License
{:license/id s/Int
:license/type (s/enum :text :link :attachment)
:license/title schema-base/LocalizedString
(s/optional-key :license/link) schema-base/LocalizedString
(s/optional-key :license/text) schema-base/LocalizedString
(s/optional-key :license/attachment-id) schema-base/LocalizedInt
(s/optional-key :license/attachment-filename) schema-base/LocalizedString
:license/enabled s/Bool
:license/archived s/Bool})
(merge
schema-base/LicenseId
{:license/type (s/enum :text :link :attachment)
:license/title schema-base/LocalizedString
(s/optional-key :license/link) schema-base/LocalizedString
(s/optional-key :license/text) schema-base/LocalizedString
(s/optional-key :license/attachment-id) schema-base/LocalizedInt
(s/optional-key :license/attachment-filename) schema-base/LocalizedString
:license/enabled s/Bool
:license/archived s/Bool}))

(s/defschema Workflow
{:id s/Int
:organization schema-base/OrganizationOverview
:title s/Str
:workflow s/Any
:licenses [License]
:enabled s/Bool
:archived s/Bool})

Expand Down
5 changes: 3 additions & 2 deletions src/clj/rems/api/services/dependencies.clj
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,9 @@

(for [workflow (workflow/get-workflows {})
dep (concat
(mapv (fn [lic] {:license/id (:id lic)}) (:licenses workflow))
(:forms (:workflow workflow))
(->> (get-in workflow [:workflow :licenses])
(mapv #(select-keys % [:license/id])))
(get-in workflow [:workflow :forms])
[(:organization workflow)])]
{:from {:workflow/id (:id workflow)} :to dep})

Expand Down
47 changes: 24 additions & 23 deletions src/clj/rems/api/services/workflow.clj
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
[rems.db.applications :as applications]
[rems.db.core :as db]
[rems.db.form :as form]
[rems.db.licenses :as licenses]
[rems.db.organizations :as organizations]
[rems.db.users :as users]
[rems.db.workflow :as workflow]
[rems.json :as json]))
[rems.db.workflow :as workflow]))

(defn invalid-forms-error [forms]
(let [invalid (seq (remove (comp form/get-form-template :form/id) forms))]
Expand All @@ -24,35 +24,35 @@
:errors [{:type :invalid-user
:users invalid}]})))

(defn create-workflow! [{:keys [organization handlers forms] :as cmd}]
(defn invalid-licenses-error [licenses]
(let [ids (map :license/id licenses)]
(when-some [invalid (seq (remove licenses/license-exists? ids))]
{:success false
:errors [{:type :invalid-license
:licenses invalid}]})))

(defn create-workflow! [{:keys [organization handlers licenses forms] :as cmd}]
(util/check-allowed-organization! organization)
(or (invalid-users-error handlers)
(invalid-forms-error forms)
(let [id (workflow/create-workflow! cmd)]
(invalid-licenses-error licenses)
(let [id (workflow/create-workflow! (-> cmd
(update :licenses #(map :license/id %))))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something that we have perhaps not specified yet in "layer architecture" is how the ids of various things should be passed on. Could be nice to have {:license/id 123} format at this level but it could also work to have the 123 for the DB layer and keep the map for API and service layers.

(dependencies/reset-cache!)
{:success (not (nil? id))
:id id})))

(defn- unrich-workflow [workflow]
;; TODO: keep handlers always in the same format, to avoid this conversion (we can ignore extra keys)
(if (get-in workflow [:workflow :handlers])
(update-in workflow [:workflow :handlers] #(map :userid %))
workflow))

;; TODO: use rems.db.workflow/edit-workflow! and don't go directly to db fns, same for other fns
(defn edit-workflow! [{:keys [id organization title handlers]}]
(let [workflow (unrich-workflow (workflow/get-workflow id))
workflow-body (cond-> (:workflow workflow)
handlers (assoc :handlers handlers))]
(defn edit-workflow! [{:keys [id organization handlers licenses] :as cmd}]
(let [workflow (workflow/get-workflow id)]
(util/check-allowed-organization! (:organization workflow))
(when organization
(util/check-allowed-organization! organization))
(db/edit-workflow! {:id id
:title title
:organization (:organization/id organization)
:workflow (json/generate-string workflow-body)}))
(applications/reload-cache!)
{:success true})
(or (invalid-users-error handlers)
(invalid-licenses-error licenses)
(let [_ (workflow/edit-workflow! (-> cmd
(update :licenses #(map :license/id %))))]
(applications/reload-cache!)
{:success true}))))

(defn set-workflow-enabled! [{:keys [id enabled]}]
(util/check-allowed-organization! (:organization (workflow/get-workflow id)))
Expand All @@ -79,8 +79,8 @@
(->> workflow
join-workflow-forms
organizations/join-organization
workflow/join-workflow-licenses
(transform [:licenses ALL] organizations/join-organization))))
(transform [:workflow :licenses ALL] (comp organizations/join-organization
licenses/join-license)))))

(defn get-workflow [id]
(->> (workflow/get-workflow id)
Expand All @@ -99,3 +99,4 @@
(get-in wf [:workflow :handlers]))
workflows)]
(->> handlers distinct (sort-by :userid))))

7 changes: 4 additions & 3 deletions src/clj/rems/api/workflows.clj
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
[rems.application.events :as events]
[rems.common.roles :refer [+admin-read-roles+ +admin-write-roles+]]
[rems.schema-base :as schema-base]
[rems.util :refer [getx-user-id]]
[ring.util.http-response :refer :all]
[schema.core :as s]))

Expand All @@ -15,14 +14,16 @@
:title s/Str
(s/optional-key :forms) [{:form/id s/Int}]
:type (apply s/enum events/workflow-types) ; TODO: exclude master workflow?
(s/optional-key :handlers) [schema-base/UserId]})
(s/optional-key :handlers) [schema-base/UserId]
(s/optional-key :licenses) [schema-base/LicenseId]})

(s/defschema EditWorkflowCommand
{:id s/Int
(s/optional-key :organization) schema-base/OrganizationId
;; type can't change
(s/optional-key :title) s/Str
(s/optional-key :handlers) [schema-base/UserId]})
(s/optional-key :handlers) [schema-base/UserId]
(s/optional-key :licenses) [schema-base/LicenseId]})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably remove the option of setting licenses in edit as it could affect existing applications. It's the same with forms and there we don't allow editing.


(s/defschema CreateWorkflowResponse
{:success s/Bool
Expand Down
9 changes: 9 additions & 0 deletions src/clj/rems/db/licenses.clj
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,12 @@
:licenses
(get-licenses {:wfid (:wfid item)
:items [(:id item)]})))

(defn join-license [x]
(-> (get-license (:license/id x))
(dissoc :id)
(merge x)))

(defn license-exists? [id]
(some? (db/get-license {:id id})))
Comment on lines +76 to +77
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could potentially be very expensive (it's a query per license). I'm sure we have some other these kinds of checks around. We could add a task to go over them and see what approach is best. Could be for example cached.


92 changes: 46 additions & 46 deletions src/clj/rems/db/test_data.clj
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
[rems.db.users :as users]
[rems.db.test-data-helpers :refer :all]
[rems.db.test-data-users :refer :all]
[rems.db.workflow :as workflow]
[rems.testing-util :refer [with-user]]
[rems.config])
(:import [java.util UUID]
Expand Down Expand Up @@ -234,75 +235,74 @@
owner (users :owner)
organization-owner1 (users :organization-owner1)
handlers [approver1 approver2 rejecter-bot]
link (create-license! {:actor owner
:license/type :link
:organization {:organization/id "nbn"}
:license/title {:en "CC Attribution 4.0"
:fi "CC Nimeä 4.0"
:sv "CC Erkännande 4.0"}
:license/link {:en "https://creativecommons.org/licenses/by/4.0/legalcode"
:fi "https://creativecommons.org/licenses/by/4.0/legalcode.fi"
:sv "https://creativecommons.org/licenses/by/4.0/legalcode.sv"}})
text (create-license! {:actor owner
:license/type :text
:organization {:organization/id "nbn"}
:license/title {:en "General Terms of Use"
:fi "Yleiset käyttöehdot"
:sv "Allmänna villkor"}
:license/text {:en (apply str (repeat 10 "License text in English. "))
:fi (apply str (repeat 10 "Suomenkielinen lisenssiteksti. "))
:sv (apply str (repeat 10 "Licens på svenska. "))}})
default (create-workflow! {:actor owner
:organization {:organization/id "nbn"}
:title "Default workflow"
:type :workflow/default
:handlers handlers})
:handlers handlers
:licenses [link text]})
decider (create-workflow! {:actor owner
:organization {:organization/id "nbn"}
:title "Decider workflow"
:type :workflow/decider
:handlers handlers})
:handlers handlers
:licenses [link text]})
master (create-workflow! {:actor owner
:organization {:organization/id "nbn"}
:title "Master workflow"
:type :workflow/master
:handlers handlers})
:handlers handlers
:licenses [link text]})
auto-approve (create-workflow! {:actor owner
:organization {:organization/id "nbn"}
:title "Auto-approve workflow"
:handlers [approver-bot rejecter-bot]})
:handlers [approver-bot rejecter-bot]
:licenses [link text]})
organization-owner (create-workflow! {:actor organization-owner1
:organization {:organization/id "organization1"}
:title "Owned by organization owner"
:type :workflow/default
:handlers handlers})
with-form (create-workflow! {:actor owner
:organization {:organization/id "nbn"}
:title "With workflow form"
:type :workflow/default
:handlers handlers
:forms [{:form/id (create-form! {:actor owner
:form/internal-name "Workflow form"
:form/external-title {:en "Workflow form"
:fi "Työvuon lomake"
:sv "Blankett för arbetsflöde"}
:organization {:organization/id "nbn"}
:form/fields [{:field/type :description
:field/title {:fi "Kuvaus"
:en "Description"
:sv "Rubrik"}
:field/optional false}]})}]})
_with-form (create-workflow! {:actor owner
:organization {:organization/id "nbn"}
:title "With workflow form"
:type :workflow/default
:handlers handlers
:licenses [link text]
:forms [{:form/id (create-form! {:actor owner
:form/internal-name "Workflow form"
:form/external-title {:en "Workflow form"
:fi "Työvuon lomake"
:sv "Blankett för arbetsflöde"}
:organization {:organization/id "nbn"}
:form/fields [{:field/type :description
:field/title {:fi "Kuvaus"
:en "Description"
:sv "Rubrik"}
:field/optional false}]})}]})
ega (create-workflow! {:actor owner
:organization {:organization/id "csc"}
:title "EGA workflow, a variant of default"
:type :workflow/default
:handlers handlers})]

;; attach both kinds of licenses to all workflows created by owner except EGA
(let [link (create-license! {:actor owner
:license/type :link
:organization {:organization/id "nbn"}
:license/title {:en "CC Attribution 4.0"
:fi "CC Nimeä 4.0"
:sv "CC Erkännande 4.0"}
:license/link {:en "https://creativecommons.org/licenses/by/4.0/legalcode"
:fi "https://creativecommons.org/licenses/by/4.0/legalcode.fi"
:sv "https://creativecommons.org/licenses/by/4.0/legalcode.sv"}})
text (create-license! {:actor owner
:license/type :text
:organization {:organization/id "nbn"}
:license/title {:en "General Terms of Use"
:fi "Yleiset käyttöehdot"
:sv "Allmänna villkor"}
:license/text {:en (apply str (repeat 10 "License text in English. "))
:fi (apply str (repeat 10 "Suomenkielinen lisenssiteksti. "))
:sv (apply str (repeat 10 "Licens på svenska. "))}})]
(doseq [licid [link text]]
(doseq [wfid [default decider master auto-approve with-form]]
(db/create-workflow-license! {:wfid wfid :licid licid}))))

{:default default
:ega ega
:decider decider
Expand Down Expand Up @@ -744,8 +744,8 @@
:more-info {:en "This DUO code is optional but recommended"}}]}}))

workflows (create-workflows! (merge users +bot-users+))
_ (db/create-workflow-license! {:wfid (:organization-owner workflows)
:licid license-organization-owner})
_ (workflow/edit-workflow! {:id (:organization-owner workflows)
:licenses [license-organization-owner]})

form (create-all-field-types-example-form! owner {:organization/id "nbn"} "Example form with all field types" {:en "Example form with all field types"
:fi "Esimerkkilomake kaikin kenttätyypein"
Expand Down
8 changes: 3 additions & 5 deletions src/clj/rems/db/test_data_helpers.clj
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,6 @@
:license/text {:fi "fi" :en "en"}
:license/attachment-id {:fi fi-attachment :en en-attachment}}))))

(defn create-workflow-licence! [wfid licid]
(db/create-workflow-license! {:wfid wfid :licid licid}))

(defn create-form! [{:keys [actor organization]
:form/keys [internal-name external-title fields]
:as command}]
Expand Down Expand Up @@ -146,7 +143,7 @@
(assert (:success result) {:command command :result result})
(:id result)))

(defn create-workflow! [{:keys [actor organization title type handlers forms]
(defn create-workflow! [{:keys [actor organization title type handlers forms licenses]
:as command}]
(let [actor (or actor (create-owner!))
result (with-user actor
Expand All @@ -157,7 +154,8 @@
:forms forms
:handlers (or handlers
(do (create-user! (get +fake-user-data+ "developer"))
["developer"]))}))]
["developer"]))
:licenses (mapv (fn [id] {:license/id id}) licenses)}))]
(assert (:success result) {:command command :result result})
(:id result)))

Expand Down
Loading