Small fixes #3232
Small fixes #3232
Conversation
This is for consistency, it is shown everywhere else.
Ideally we would use `text-format` so that the parens and colon would come from the localization as well, but this is a clear improvement.
When copying a form, the copied form retains the organization (old) and the user may not be member of it. If the organization is not changed in the dropdown it used to persist and cause Forbidden error. Now we verify that the organization of an item is valid, or reset it otherwise. This allows user to select it after a simple validation fail.
This caused warning e.g. when autosave happens on a freshly opened application.
| (defn organization-field [context {:keys [keys readonly on-change]}] | ||
| (let [label (text :t.administration/organization) | ||
| organizations @(rf/subscribe [:owned-organizations]) | ||
| (let [id (keys-to-id keys) | ||
| label (text :t.administration/organization) | ||
| owned-organizations @(rf/subscribe [:owned-organizations]) | ||
| valid-organizations (->> owned-organizations (filter :enabled) (remove :archived)) | ||
| disallowed (roles/disallow-setting-organization? @(rf/subscribe [:roles])) | ||
| language @(rf/subscribe [:language]) | ||
| form @(rf/subscribe [(:get-form context)]) | ||
| value (get-in form keys) | ||
| potential-value (get-in form keys) | ||
| on-change (or on-change (fn [_])) | ||
| wrapped-on-change #(let [new-value %] | ||
| (rf/dispatch [(:update-form context) keys new-value]) | ||
| (on-change new-value)) | ||
|
|
||
| ;; if item was copied then this org could be something old | ||
| ;; where we have no access to so reset here | ||
| value (if (or readonly | ||
| disallowed | ||
| (contains? (set (mapv :organization/id valid-organizations)) | ||
| (:organization/id potential-value))) | ||
| potential-value | ||
|
|
||
| ;; not accessible, reset | ||
| (wrapped-on-change nil)) |
There was a problem hiding this comment.
reflecting on earlier discussion: i noticed that copy as new does fetch form template from API, and it got me thinking that would it make sense to include extra param and have API remove the disallowed organization? this could make UI code be less complected wrt. disallowed organization, although extra parameter smells a lot like backend for frontend. existing API would be unconcerned with it though.
related implementation thoughts:
;; new function in rems.service.form
(defn get-form-for-copy [form]
(let [organization (rems.db.organizations/get-organization-by-id-raw
(get-in form [:organization :organization/id]))]
(if (rems.service.util/may-edit-organization? organization)
form
(dissoc form :organization))))
;; amend rems.api.forms with query-params
(GET "/:form-id" []
:summary "Get form by id"
:roles +admin-read-roles+
:path-params [form-id :- (describe s/Int "form-id")]
:query-params [{copy :- (describe s/Bool "whether to format return value suitable for copy-as-new action") false}]
:return schema/FormTemplate
(if-let [form (form/get-form-template form-id)]
(ok (cond-> form
copy form/get-form-for-copy))
(not-found-json-response)))
;; add query-param in rems.administration.create-form/enter-page event-fx fetcher
:dispatch-n [(when form-id [::form {:copy true}])] There was a problem hiding this comment.
I think there could be two different API endpoints, one for copy like /forms/123/copy, and choose which one to call.
There was a problem hiding this comment.
Since we don't have this kind of functionality in the API (copy), and it doesn't make as much sense in the API (you can just get the regular form and then do whatever in the client), I'm hesitant to add it like this. If it were a pure client action copy then it'd make sense, however that does not make the code any simpler. It just moves the logic to the fetcher, which maybe needs to have the subscription data injected to it because it's not a component.
There was a problem hiding this comment.
OK, sounds reasonable. here i don't see added value in moving more logic to fetcher, compared to doing filtering in organization component.
having a common way to filter domain objects in client, such that it is easy to present only e.g. own organization forms could be good for maintainability. this could be achieved by refactoring entities to use same patterns like [:organization :organization/id]. enriching API return values with ACL could provide similar benefits, however it is quite a bit heavier to implement.
There was a problem hiding this comment.
We should try to unify the domain objects a bit first. And perhaps this filter can try first :organization/id and then :organization :organization/id somewhat automatically. That's basically what the pattern could be after unifying the domain objects a bit better.
Close #2880
Close #3230
:and parens too.:loading?attribute being passed to HTML where it has invalid meaning.Checklist for author
Remove items that aren't applicable, check items that are done.
Reviewability
Documentation
Testing
Follow-up