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.

Sign up for GitHub

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

Jump to bottom

usability: links to methods and configs [risk: low] #1444

Merged
merged 9 commits into from
Nov 6, 2018
Merged

usability: links to methods and configs [risk: low] #1444

merged 9 commits into from
Nov 6, 2018

Conversation

davidangb
Copy link
Contributor

@davidangb davidangb commented Nov 2, 2018 •
edited
Loading

DataBiosphere/firecloud-app#111

For usability, this PR adds linkifies a few parts of the UI:

  1. When viewing a single submission, or a single workflow inside a submission, link to the workspace's method config on which the submission ran.
  2. When viewing a single method config inside a workspace, link to the referenced Agora method, if it has not been redacted. (no links for dockstore)
  3. When viewing the list of method configs inside a workspace, link to their referenced Agora methods, if they have not been redacted. (no links for dockstore)

I chose to NOT add links from the workspace's list of submissions back to each referenced config. To do this right, I'd have to make an extra ajax call to get all known configs, in order to handle configs that have been updated/deleted since the submission ran. This would be an extra performance hit. If this feature is really desired, I'd do it in a separate PR.

Workspace method config detail (snapshot id value is a link):
config-detail

Workspace method config list:
config-list

Submission detail, config unavailable:
submission-detail-deleted

Submission detail, config reachable:
submission-detail-active

  • Submitter: Include the JIRA issue number in the PR description
  • Submitter: Check documentation and code comments. Add explanatory PR comments if helpful.
  • Submitter: If you changed a URL that is used elsewhere (e.g. in an email), comment about where it is used and ensure the dependent code is updated.
  • Submitter: JIRA ticket checks:
    • Acceptance criteria exists and is met
    • Note any changes to implementation from the description
    • To Demo flag is set
    • Release Summary is filled out, if applicable
    • Add notes on how to QA
  • Submitter: Update RC_XXX release ticket with any config or environment changes necessary
  • Submitter: Update FISMA documentation if changes to:
    • Authentication
    • Authorization
    • Encryption
    • Audit trails
  • Submitter: If you're adding new libraries, sign us up to security updates for them
  • Submitter: If you're adding new automated UI tests, review the test plan with QA
  • Review cycle:
    • LR reviews
    • Rest of team may comment on PR at will
    • LR assigns to submitter for feedback fixes
    • Submitter rebases to develop again if necessary
    • Submitter makes further commits. DO NOT SQUASH
    • Submitter updates documentation as needed
    • Submitter reassigns to LR for further feedback
  • TL sign off
  • LR sign off
  • Product Owner sign off
  • Submitter: Verify all tests go green, including CI tests and automated UI tests.
  • Submitter: Squash commits and merge to develop. If adding test code, merge application code and test code at the same time.
  • Submitter: Delete branch after merge
  • Submitter: Test this change works on dev environment after deployment. YOU own getting it fixed if dev isn't working for ANY reason!
  • Submitter: Mark JIRA issue as resolved once this checklist is completed

@davidangb
Copy link
Contributor Author

davidangb commented Nov 4, 2018 •
edited
Loading

first automation run had two failures:

[info] Run completed in 26 minutes, 43 seconds.
[info] Total number of tests run: 123
[info] Suites: completed 27, aborted 0
[info] Tests: succeeded 121, failed 2, canceled 0, ignored 7, pending 0
[info] *** 2 TESTS FAILED ***
[error] Failed tests:
[error] 	org.broadinstitute.dsde.firecloud.test.security.AuthDomainMultiGroupsSpec
[error] 	org.broadinstitute.dsde.firecloud.test.metadata.DataDownloadSpec
[error] (Test / testOnly) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 1656 s, completed Nov 2, 2018 6:09:24 PM

MattPutnam
MattPutnam approved these changes Nov 5, 2018
View reviewed changes
src/cljs/main/broadfcui/page/workspace/workspace_common.cljs Outdated
@@ -47,6 +48,23 @@
(defn config->id [config]
(select-keys config [:namespace :name]))

(defn- referenced-method-parts [config]
((juxt :sourceRepo :methodNamespace :methodName :methodPath :methodVersion) (:methodRepoMethod config)))
Copy link
Contributor

Choose a reason for hiding this comment

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

This strikes me as a little weird. Maybe:

(replace (:methodRepoMethod config) [:sourceRepo :methodNamespace :methodName :methodPath :methodVersion])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for this - the juxt was cut-paste from the previous table definition and I also found it weird but didn't investigate anything better - I like your suggestion a lot more!

src/cljs/main/broadfcui/page/workspace/workspace_common.cljs Outdated
((juxt :sourceRepo :methodNamespace :methodName :methodPath :methodVersion) (:methodRepoMethod config)))

(defn- method-as-text [config]
(clojure.string/join "/" (referenced-method-parts config)))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: import clojure.string :as string and then string/join

zarsky-broad
zarsky-broad reviewed Nov 6, 2018
View reviewed changes
src/cljs/main/broadfcui/components/entity_details.cljs Outdated
@@ -78,7 +79,12 @@
"agora" (list
(make-field :namespace "Namespace")
(make-field :name "Name")
(make-field :snapshotId "Snapshot ID" :dropdown? true))
(make-field :snapshotId "Snapshot ID" :dropdown? true
:render (fn [snapshotId]
Copy link
Contributor

Choose a reason for hiding this comment

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

Kebab case the param

src/cljs/main/broadfcui/page/workspace/monitor/submission_details.cljs Outdated
[:span {:style {:fontWeight 500}} (:methodConfigurationName submission)]])
;; if we were able to retrieve the method config (see :load-details), then display
;; the config's name as a link. Else, display it as text with an informative tooltip.
(if (:ws-config @state)
Copy link
Contributor

Choose a reason for hiding this comment

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

Destructure state and props

Copy link
Contributor

Choose a reason for hiding this comment

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

Also wrap this in a let and destructure the things you're using from submission

src/cljs/main/broadfcui/page/workspace/workspace_common.cljs Outdated
(string/join "/" (referenced-method-parts config)))

(defn- method-as-maybe-link [config]
(let [redacted? (:redacted? config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use :keys to destructure multiple at once

src/cljs/main/broadfcui/page/workspace/workspace_common.cljs Outdated
:sort-by :text
:as-text method-as-text
:render method-as-maybe-link
}]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Move these up a line

@davidangb davidangb merged commit 16dcd0b into develop Nov 6, 2018
@davidangb davidangb deleted the da_111_linkToMethodsConfigs branch November 6, 2018 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MattPutnam MattPutnam MattPutnam approved these changes

@zarsky-broad zarsky-broad zarsky-broad approved these changes

Assignees

@zarsky-broad zarsky-broad

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@davidangb @MattPutnam @zarsky-broad