-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add auto DOI opt-out toggle for legacy and hosted workflows #2059
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## hotfix/2.13.4 #2059 +/- ##
=================================================
- Coverage 41.84% 41.81% -0.04%
=================================================
Files 394 394
Lines 12321 12330 +9
Branches 2959 2960 +1
=================================================
Hits 5156 5156
- Misses 4845 4855 +10
+ Partials 2320 2319 -1 ☔ View full report in Codecov by Sentry. |
@@ -171,7 +171,7 @@ <h3> | |||
class="private-btn small-btn-structure ml-2" | |||
matTooltip="Manage DOIs" | |||
(click)="manageDois()" | |||
[disabled]="!canWrite" | |||
[disabled]="!canWrite || (isRefreshing$ | async)" |
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.
I noticed this button is not disabled when the page is loading after an action (ex: Publish) so I just added this. Same thing for the Manage labels button below
*ngIf="!isGitHubAppEntry" | ||
[(ngModel)]="isAutoDoiEnabled" | ||
(change)="toggleAutoDoiGeneration()" | ||
labelPosition="before" |
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.
Recommend changing to "after"
so it matches the positioning in the mockups (after the slider), just in case anyone was attached to that particular positioning.
this.workflowsService.autoGenerateDois(entry.id, { autoGenerateDois: isAutoDoiEnabled }).subscribe( | ||
(response) => { | ||
this.alertService.detailedSuccess(); | ||
this.workflowService.setWorkflow({ ...this.workflowQuery.getActive(), autoGenerateDois: response }); |
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.
After successfully toggling the slider, the "success" snackbar pops up below, but if you try to click "Dismiss" within the snackbar, the Manage DOIs dialog is dismissed. Also, when the "success" snackbar pops up, it's little distracting, imho. Would it be ok to simply not display it (on "success")? (Given that the mechanics of the "which doi" radio box have changed, and same thing happens when a radio box is clicked, same question for it, also.)
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.
Changed it to simpleSuccess()
so the snackbar doesn't pop up
> | ||
</span> | ||
<span *ngIf="isGitHubAppEntry"> | ||
Edit the enableAutoDoi field in the .dockstore.yml to change your Automatic DOI Generation preference. |
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.
In the isGitHubAppEntry
true version, there are two "info" icons in this block of text. By itself, probably not a problem, but the spacing between the two icons and this text passage is equidistant, so it's hard to tell which icon belongs to which text passage. A little additional spacing on the left side of this text might help to set it off from the first icon, and group it better with the second...
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.
Agreed, this was something I played around with for a bit and I wasn't completely satisfied with it.
How about instead, we add some new documentation to https://docs.dockstore.org/en/stable/end-user-topics/dois.html#automatic-dockstore-doi-generation about how to disable automatic DOIs? Then I can just keep a single info icon at the end of the description that links to https://docs.dockstore.org/en/stable/end-user-topics/dois.html#automatic-dockstore-doi-generation which contains all the needed info for automatic DOIs?
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.
I went with this approach so there's only one info icon. I created https://ucsc-cgl.atlassian.net/browse/SEAB-6950 to update our documentation to include instructions on opting out
In the screenshot, does the toggle disappear when set to On? |
No, the first screenshot is for a legacy workflow which has a toggle and the second screenshot is for a GitHub App workflow which has no toggle. I'll update the description to be clearer 🙂 |
Thanks for clarifying. Looks good! |
|
Description
I recommend reviewing this with "Hide whitespace" checked.
This PR adds a Automatic DOI Generation toggle to the Manage DOIs dialog so that users can opt out of auto DOI generation for legacy and hosted workflows. GitHub App workflow, tools, notebooks, and services also have this section except the toggle is not displayed (the ON/OFF status is still displayed though). There is a sentence telling the users to edit the .dockstore.yml, which is similar to what we do for the GitHub App manual topic.
Based on UI best practices, the slide toggle takes effect immediately if pressed and does not require the user to click Save to apply the new state. The Manage DOI dialog has radio buttons for DOI selection, and it feels like a confusing experience to have the user click Save to save this selection while the toggle takes effect immediately. Thus, I changed it so that the radio button selection is saved immediately when pressed. The Save button is now just a Done button that closes the dialog.
Note this PR requires dockstore/dockstore#6074 which allows authenticated users to call the
PUT autodoigeneration
endpoint to toggle the auto DOI setting.Before
After
Manage DOIs dialog for legacy and hosted workflows have a slide toggle:

Manage DOIs dialog for GitHub App entries do not have a toggle. It displays On/Off and tells the user to edit the .dockstore.yml:

Review Instructions
On staging, for a legacy or hosted workflow, opt out of DOI selection by clicking Manage DOIs and clicking the toggle. Refresh the page and verify that the setting is Off.
For a GItHub App entry, click the Manage DOIs button and verify that the Automatic DOI Generation section displays On or Off without a slide toggle. Verify that it tells the user to edit their .dockstore.yml.
Issue
https://ucsc-cgl.atlassian.net/browse/SEAB-6807
Security
If there are any concerns that require extra attention from the security team, highlight them here.
Please make sure that you've checked the following before submitting your pull request. Thanks!
npm run build
markdown-wrapper
component, which does extra sanitizationnpm audit
and ensure you are not introducing new vulnerabilities