Y25-286 - Add study accessioning checklist #5499
Conversation
StephenHulme
added
UX
StephenHulme
changed the title
This is better practise in Rails
…dy-accessioning-checklist
StephenHulme
requested review from
BenTopping,
andrewsparkes,
dasunpubudumal,
neilsycamore and
yoldas
andrewsparkes
left a comment
andrewsparkes
left a comment
There was a problem hiding this comment.
Looks good other than the minor comment about double negatives.
I think it needs changing as it's confusing to understand.
| <% if @study.study_metadata.strategy_not_applicable? %> | ||
| <%= bad_icon %> Study release strategy is <strong><%= Study::DATA_RELEASE_STRATEGY_NOT_APPLICABLE.humanize %></strong> - <%= link_to('Change release strategy', edit_study_path(@study)) if can?(:edit, @study) %> | ||
| <% else %> | ||
| <%= good_icon %> Study release strategy is not <%= Study::DATA_RELEASE_STRATEGY_NOT_APPLICABLE.humanize %> <%= tag.strong { "(#{@study.study_metadata.data_release_strategy.humanize})" } %> |
There was a problem hiding this comment.
Noticed in the screenshots there were double negatives. Like 'is not Not applicable' and 'is not Never'.
Consider different wording?
There was a problem hiding this comment.
maybe 'cannot be' instead of 'is not'?
There was a problem hiding this comment.
or 'should not be'
still not great for clarity, anyway you get my point
There was a problem hiding this comment.
Good suggestion, thanks.
I've changed the wording to focus on what the current study settings are, rather than what they are not.
|
Thanks for the suggestions @andrewsparkes I've updated the wording and formatting as below: Happy study: Sad study without user permissions: Sad study with user permissions: |
|
|
||
| def dac_accession | ||
| @study = Study.find(params[:id]) | ||
|
|
There was a problem hiding this comment.
Does this need an accessioning_enabled guard clause?
There was a problem hiding this comment.
Added and refactored
|
|
||
| def policy_accession | ||
| @study = Study.find(params[:id]) | ||
|
|
There was a problem hiding this comment.
Does this need an accessioning_enabled guard clause?
There was a problem hiding this comment.
Yip, added and refactored
| end | ||
|
|
||
| def fresh_sevice_link | ||
| def fresh_service_link |
|
I've made some small UX changes, including updating the warning message on @neilsycamore's request: 
@andrewsparkes and @BenTopping would you please take another look? |
Closes #5417
Merges into #5486
See #5500 for a related accessioning permissions issue that was discovered as part of this work.
Changes proposed in this pull request
Provides vital feedback on the accession-ability of studies and samples.
Accession all sampleslink if user is not permitted to accessionAccessioningDisabledErrorwithaccessioning_enabled?fresh_sevice_linktypoaccessioning_enabled?- a single, global accessioning-enabled check, same asconfigatron.accession_samplespermitted_to_accession?- an explicit check for user permission to accession, same ascan?(:accession, object)Study#samples_accessionable?- does a study meet eligibility criteria for accessioning samplesSample#should_be_accessioned?- does a sample meet eligibility criteria for being accessionedScreenshots
Instructions for Reviewers
[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to
main]- Check story numbers included
- Check for debug code
- Check version