DOCK-1195: Add archived entry UI #1852
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1852 +/- ##
===========================================
- Coverage 40.53% 40.47% -0.06%
===========================================
Files 365 366 +1
Lines 11283 11359 +76
Branches 2896 2913 +17
===========================================
+ Hits 4574 4598 +24
- Misses 4407 4449 +42
- Partials 2302 2312 +10
☔ View full report in Codecov by Sentry. |
svonworl
requested review from
kathy-t,
coverbeck,
hyunnaye,
denis-yuen and
david4096
|
In your PR description, the link to the GitHub issue is missing dockstore/dockstore (I've made the same mistake many times :) ). |
src/app/entry/archive/dialog/archive-entry-dialog.component.html
Outdated
Show resolved
Hide resolved
src/app/entry/archive/dialog/archive-entry-dialog.component.html
Outdated
Show resolved
Hide resolved
|
|
||
| constructor( | ||
| public dialogRef: MatDialogRef<ArchiveEntryDialogComponent>, | ||
| @Inject(MAT_DIALOG_DATA) public data: { entry: Entry; callback: (archived: Entry) => void }, |
There was a problem hiding this comment.
Despite our attempts with the OpenAPI hierarchy, DockstoreTool and Workflow unfortunately do not extend Entry. If they did extend Entry, this would be the right way.
Since they don't, I'd suggest making this entry: Workflow | DockstoreTool.
Rationale: if we ever introduce a new Entry that doesn't extend DockstoreTool or Workflow, unlikely though that may be, we would get a compilation error, which we would want, because line 46 would fail at runtime.
That said, you have to go through hoops to enforce type checking with MAT_DIALOG_DATA, which we don't do, so I guess optional.
There was a problem hiding this comment.
I get your drift, the two approaches have different pros and cons, for sure. Yes, Workflow and DockstoreTool don't formally derive to Entry right now, but we know they are derivatives. The choice to use Entry was intentional on my part, so it's best IMHO, but you're the lead, so feel free to override... :)
As a compromise, I added some code to line 46 that uses the Entry.gitUrl as a fallback, so for a radically-different type of new entry, it's more likely to display something moderately useful, at least...
| title="{{ toolObj?.name + (toolObj?.toolname ? '/' + toolObj?.toolname : '') }}" | ||
| [routerLink]="'/my-tools/' + toolObj.tool_path" | ||
| > | ||
| {{ toolObj?.name + (toolObj?.toolname ? '/' + toolObj?.toolname : '') }} |
There was a problem hiding this comment.
The de-duplication is great 😀 if you want to take it one step further, I found this pipe today that does what this line and a couple line above does
| title="{{ workflowObj?.repository + (workflowObj?.workflowName ? '/' + workflowObj?.workflowName : '') }}" | ||
| [routerLink]="'/my-' + (entryType$ | async) + 's/' + workflowObj.full_workflow_path" | ||
| > | ||
| {{ workflowObj?.repository + (workflowObj?.workflowName ? '/' + workflowObj?.workflowName : '') }} |
There was a problem hiding this comment.
Ditto about the pipe
src/app/entry/archive/dialog/archive-entry-dialog.component.html
Outdated
Show resolved
Hide resolved
|
I addressed the feedback, the most visible changes are edits to the dialog text, and the addition of a yellow banner above an archived entry (the message text is pretty much the same as github's, it says the two important things). The text in the banner is left-aligned because given the location of the prominent items on the entry page, there's probably more of a chance that the user will read it. The date format is one of the several we use... I've re-requested review. Screenshots below: 
|
|
SonarCloud Quality Gate failed.
|
Description
This PR modifies the UI to accommodate archived entries, which are intended to be read-only and deemphasized in the UI. We imagine that users will archive old/retired/obsolete entries that they aren't allowed to delete. To that end, this PR:
See below for a screenshot.
In retrospect, this should have been split into at least two PRs, but by the time I realized so, it was too late.
Editorial: this PR reminded me of how very difficult it can be to correctly make substantial changes to the current UI. There's many contributing factors: the relative heaviness of Angular, the tool/workflow split, extensive Observable manipulation, state scattered everywhere, race conditions, lack of documentation, duplicate+ low-level code/HTML, etc. In many systems, relative to the backend, it's much easier to experiment with and iterate the UI, but not here. In the longer term, it'd probably benefit us to consider how to improve the situation. Retire the old-style tool concept? Simplify the UI to simplify the code? Refactor (to varying degrees)? UI3? IMHO, all of those options should be on the table...
In that vein, there are numerous slight inconsistencies between how the new archive UI appears/works for workflows/notebooks/apptools versus old-style tools, much like the existing UI. :)
Screenshots
The gate dialog:
The "my" page an archived workflow:
Review Instructions
Register and publish an entry on qa, then navigate to the entry page and archive it. Confirm that when the entry is archived, the entry is marked "Archived", the UI becomes read-only, the entry appears in the "Archived" section of your entry sidebar, and that the entry appears last in the appropriate Search results. Unarchive the entry and confirm that all of these changes have been reversed.
Issue
https://ucsc-cgl.atlassian.net/browse/DOCK-1195
dockstore/dockstore#3199
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 buildmarkdown-wrappercomponent, which does extra sanitizationnpm auditand ensure you are not introducing new vulnerabilities