-
Notifications
You must be signed in to change notification settings - Fork 21
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
Workflow licenses #2973
Conversation
Previously workflow licenses have existed (as legacy feature) in workflow_licenses table as id,wfid,licid columns. As part of effort to unify code conventions the workflow_licenses table is due for removal, and the data needs to be migrated into workflow table under workflowBody json column.
Migration to remove workflow_licenses table is done separately from data migration for simpler SQL. Removing table would break integration tests awfully if down migration was missing to restore the removed table.
Fetching catalogue item and workflow licenses was previously handled by a single SQL query. This is now separated, so rems.db.applications/get-catalogue-item-licenses relies on rems.db.workflow/get-workflow to enrich licenses.
9cd64fd
to
d6577e6
Compare
038c595
to
262551e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, sorry for the review taking a while.
Also based on looking at the UI, I think there's a couple small points to address here or if you can create a new task:
-
Could we disable editing of the licenses for workflows? We don't allow the editing of forms, because this could affect existing applications. A better version would allow editing of these if the workflow is not in use yet (not in use in any application or so).
-
The margins / spacing in these Licenses looks a bit off. Perhaps the same with the Forms. There is extra margin compared to e.g. Handlers, and also slightly more in bottom than top?
(let [id (workflow/create-workflow! (-> cmd | ||
(update :licenses #(map :license/id %))))] |
There was a problem hiding this comment.
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.
src/clj/rems/api/workflows.clj
Outdated
|
||
(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]}) |
There was a problem hiding this comment.
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.
(defn license-exists? [id] | ||
(some? (db/get-license {:id id}))) |
There was a problem hiding this comment.
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.
@@ -0,0 +1,7 @@ | |||
CREATE TABLE workflow_licenses ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be IF NOT EXISTS? 🤔
src/cljc/rems/common/util.cljc
Outdated
@@ -608,3 +608,12 @@ | |||
(is (= "id_123-element" (escape-element-id "123-element"))) | |||
(is (= "id__123-element" (escape-element-id " 123-element"))))) | |||
|
|||
(defn keep-keys [f coll] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A docstring would be good for a utility fn?
src/clj/rems/db/applications.clj
Outdated
workflow/get-workflow | ||
(get-in [:workflow :licenses]))] | ||
(->> (licenses/get-licenses {:items [catalogue-item-id]}) | ||
(map #(keep-keys {:id :license/id} %)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we do this and (map #(select-keys ...))
quite much, there might be a place for some helper that contains the map
in it too. The regular keep
works on sequences so should this keep-keys
too? And it could do the map
?
:handlers (get workflow :handlers)}))) | ||
:handlers (get workflow :handlers) | ||
:licenses (->> (:licenses workflow) | ||
(map #(replace-key % :license/id :id)))}))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe here also, something like replace-keys
that maps
and contains a map to potentially rename multiple keys?
relates #2158
Add support for creating/editing licenses into workflows. Workflow licenses have existed before in some form (
workflow_licenses
table), so this needed some migration work. Application model enrichment is updated to use workflow licenses.some larger changes:
workflow_licenses
table and related queriesworkflow
table json columnapi changes:
licenses
from workflow (it is now under[:workflow :licenses]
)feature:
Checklist for author
Remove items that aren't applicable, check items that are done.
Reviewability
Backwards compatibility
Documentation
Testing
Follow-up
Screenshots
edit workflow
![Screenshot 2022-09-06 at 16 42 33](https://user-images.githubusercontent.com/794087/188650704-f860f778-1a2c-432b-844c-85d032d5eafa.png)