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

Licenses with attachments #893

Merged
merged 42 commits into from Feb 7, 2019

Conversation

Projects
None yet
4 participants
@Kauko
Copy link
Collaborator

Kauko commented Jan 31, 2019

Closes #808

Kauko added some commits Jan 28, 2019

Add endpoints for adding and removing license attachments
When creating a localized license, add an optional
attachmend id.
Fix saving attachment-id to new license
Destructured poorly, didn't work.

Also add attachment-id to localized license when fetching from db.
Add attachment-id to non-localised version of license too
This way it works the same way as title and textcontent.

@Kauko Kauko changed the title WIP: Licenses with attachments Licenses with attachments Feb 1, 2019

@opqdonut
Copy link
Collaborator

opqdonut left a comment

it's nice

some comments / questions

Show resolved Hide resolved src/clj/rems/api/licenses.clj Outdated
Show resolved Hide resolved src/clj/rems/api/licenses.clj
Show resolved Hide resolved test/clj/rems/test/api/licenses.clj
Show resolved Hide resolved src/clj/rems/db/test_data.clj Outdated
Show resolved Hide resolved resources/translations/en.edn Outdated
Show resolved Hide resolved resources/translations/fi.edn Outdated
Show resolved Hide resolved src/clj/rems/api/licenses.clj
Show resolved Hide resolved src/clj/rems/api/licenses.clj
@Macroz
Copy link
Collaborator

Macroz left a comment

UI looks fine to me. I would add a field name to the type options group.

" "
(str/upper-case (:langcode localization)))
[attachment-link (:attachment-id localization) (:title localization)]
{:no-box? true}])))

This comment has been minimized.

@foxlynx

foxlynx Feb 6, 2019

Member

Could all the new code above be unified with resource licenses if you gave the text as a parameter to a function that both would use?

This comment has been minimized.

@Macroz

Macroz Feb 6, 2019

Collaborator

Yeah that's what the TODO is there for (before the fn). It's unclear at the moment if resource, workflow or form licenses all are needed, and if they are all equal. Also the structures that our API returns are currently not identical so they should be unified. This however is quite a lot of work that I'd rather do later when the first questions are resolved.

This comment has been minimized.

@foxlynx

foxlynx Feb 7, 2019

Member

I was thinking just something like this:

(defn create-license-fields [info-text entity]
  (for [localization (:localizations entity)]
                    (when (:attachment-id localization)
                      [inline-info-field
                       (str info-text " " (str/upper-case (:langcode localization)))
                       [attachment-link (:attachment-id localization) (:title localization)]
                       {:no-box? true}])))

....
(when (= "attachment" (:licensetype license))
    (create-license-fields (text :t.create-license/license-attachment) license)
...

@foxlynx

foxlynx approved these changes Feb 7, 2019

@Macroz Macroz merged commit 85d3ca8 into master Feb 7, 2019

7 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: cloverage Your tests passed on CircleCI!
Details
ci/circleci: doo Your tests passed on CircleCI!
Details
ci/circleci: ok Your tests passed on CircleCI!
Details
ci/circleci: war Your tests passed on CircleCI!
Details
ci/circleci: without-db Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Macroz Macroz deleted the 808_attachment_licenses branch Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment