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.

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

Annotation and Title detail #18762

Open
wants to merge 14 commits into
base: dev
Choose a base branch
from

Conversation

hujambo-dunia
Copy link
Contributor

@hujambo-dunia hujambo-dunia commented Sep 3, 2024

Addresses #18711. Visually, there are numerous ambiguous ways to solve this issue.

Version 3 takes into account Marius' comment for simplicity but maintaining consistency with Workflows Landing Page design_.

v3: Expandable Portlet component with site consistency for Workflows Landing Page

demov3_Expandable_Portlet_Site_Consistency_for_Workflows_Landing


Version 2 takes into account Ahmed's comment for consistency with Tool Form page header design.

v2: Expandable Portlet component with site consistency

demov2_Expandable_Portlet_Site_Consistency


Version 1 is illustrated below as an "expandable portlet" solution. Other ideas/possibilities: "Mouse Hover Overlay-Menu", "Mouse Click Overlay-Menu", "Only Display Description", and so forth. After general solution is agreed, will complete "code clean-up" and test script fix process. Constructive ideas/comments welcome.

v1: Expandable Portlet component
demov1_Expandable_Portlet

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@hujambo-dunia hujambo-dunia changed the title New component for Title and Annotation detail Annotation and Title detail Sep 3, 2024
@hujambo-dunia
Copy link
Contributor Author

@ElectronicBlueberry this commit will have negative downstream effects?

@ahmedhamidawan
Copy link
Member

ahmedhamidawan commented Sep 3, 2024

I know this is WIP, but one tiny styling improvement could be made by placing the run and cog button in the same line as the wf name:

image

as opposed to:

image

One way to achieve this would be a <slot> for workflow-run-settings (and the run button) maybe?


On a somewhat related note, I always felt that using the same styled header as a ToolCard for workflows could also look nice?:

image

Kind of like:

image

@hujambo-dunia hujambo-dunia marked this pull request as ready for review September 10, 2024 14:56
@github-actions github-actions bot added this to the 24.2 milestone Sep 10, 2024
client/src/components/Workflow/Run/WorkflowRunName.vue Outdated Show resolved Hide resolved
workflowInfoData.value = await workflowInfoDataPromise;
} catch (e) {
const error = e as AxiosError<{ err_msg?: string }>;
messageVariant.value = "danger";
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like messageVariant will only be used for "danger" when there is an error, so you can fix the variant to danger and safely remove the redundant ref.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davelopez very helpful comment - change made.

Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Really cool, thank you! But please don't embed the published card there. Just a simple expand would be brilliant.

const expandAnnotations = ref(true);
const workflowInfoData = ref(null);

onMounted(() => {
Copy link
Member

Choose a reason for hiding this comment

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

onMounted runs after initial rendering and creation of the DOM. You don't need to wait for that.

loading.value = true;

try {
const workflowInfoDataPromise = getWorkflowInfo(props.model.runData.id);
Copy link
Member

Choose a reason for hiding this comment

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

This bypasses all stores and fetches the workflow even if we already have it. Can you use useWorkflowInstance instead ?

An even better alternative is to serialize the workflow annotation for the run request. This would be as simple as

diff --git a/lib/galaxy/managers/workflows.py b/lib/galaxy/managers/workflows.py
index 0d970de242..5921ad74f4 100644
--- a/lib/galaxy/managers/workflows.py
+++ b/lib/galaxy/managers/workflows.py
@@ -1051,6 +1051,7 @@ class WorkflowContentsManager(UsesAnnotations):
             step_models.append(step_model)
         return {
             "id": trans.app.security.encode_id(stored.id),
+            "annotation": stored.annotation,
             "history_id": trans.app.security.encode_id(history.id) if history else None,
             "name": stored.name,
             "owner": stored.user.username,

we only use the run endpoint here in the workflow run component, so if we always need the attribute I don't see a reason not to just serialize that as well.

You would then have this available in the runData object here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up questions related to the existing _workflow_to_dict_run() method in Workflows.py (eg. maybe some refactoring needed as a separate ticket).

(1) Blocker: I am assuming the purpose of @mvdbeek 's code suggestions is to not make an additional database call, correct? However, using the VSCode Debugger to find already-available values, I have not yet been able to find the following variables for the WorkflowInformation.vue component to be fully displayed. Specifically, creator, email_hash. and license. Although, the License may not be appearing because there is no value for it in my dev-qa use-case, but email_hash should display (since there is an avatar selected) and creator should always have a value. I am assuming that Email Hash can be created from the non-hashed email string parameter: stored.annotations[0].stored_workflow.user.email? I was able to find/serialize the other values (including Workflow Tags).
(2) Side note: calling stored.annotation (singular) fails with the error: 'StoredWorkflow' object has no attribute 'annotation'. The plural value can be accessed using stored.annotations ; however, it lacks parameters like the Workflow Tags. Instead, I used stored.annotations[0].stored_workflow to get the Workflow Tags - if that is an unreliable value, please let me know.
(3) Side note: using useWorkflowInstance pulls up an entirely different Workflow when using the Encoded Workflow ID (that appears in the URL). Tried also with the literal Workflow ID number (as a string) and failed. Perhaps this works only for Invocations since that is where I see useWorkflowInstance used in the codebase currently...? The method seemed to be able to used successfully in WorkflowInvocationHeader.vue so hopefully @ahmedhamidawan can please advise on this question?
(4) Question: Not a blocker, but I was surprised the following element didn't contain Workflow Annotation data within the same method: workflow.annotation returns null instead of any actual Workflow Annotation data. If that is not expected, then this line is broken. If "null" is expected, how can this be -is the variable misnamed? This code block is pretty old, but perhaps last editors have some thoughts: @guerler @nsoranzo @dannon ?

Copy link
Member

Choose a reason for hiding this comment

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

useWorkflowInstance takes the stored workflow_id as parameter, not the encoded_workflow_id we have here from the run data. The stored id is essentially the id for the specific workflow (for the given version; latest in this case) retrieved here:

workflow = stored.get_internal_version(version)

Once we have that id, the whole loadAnnotation method could be replaced by a:

const { workflow, loading, error } = useWorkflowInstance(stored_workflow_id);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahmedhamidawan Thank you so much for this helpful comment. The latest iteration of this (v3) doesn't need this additional data right now, but I may need to know this helpful piece of information in the future.

if step.annotations:
step_model["annotation"] = step.annotations[0].annotation
if step.upgrade_messages:
step_model["messages"] = step.upgrade_messages
step_models.append(step_model)
return {
"id": trans.app.security.encode_id(stored.id),
"annotation": annotations_dict,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mvdbeek Please let me know if it would be even better to serialize these SQLAlchemy objects in stored.annotations in some other way, happy to replace.

name: props.model.runData.name,
owner: props.model.runData.owner,
tags: props.model.runData.annotation.tags.map((t: { user_tname: string }) => t.user_tname),
annotations: [props.model.runData.annotation.annotation],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@itisAliRH for this line annotations: [props.model.runData.annotation.annotation], is ok to only use the first annotation record (ie. annotation[0])? Or are other annotation records need to be included/consolidated somehow?

@hujambo-dunia
Copy link
Contributor Author

@itisAliRH Wanted your review on changes to the WorkflowCard.vue file in this commit modifies your component to include a show-actions Boolean props

if step.annotations:
step_model["annotation"] = step.annotations[0].annotation
if step.upgrade_messages:
step_model["messages"] = step.upgrade_messages
step_models.append(step_model)
return {
"id": trans.app.security.encode_id(stored.id),
"annotation": annotations_dict,
Copy link
Member

Choose a reason for hiding this comment

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

You're including properties here that are not part of the annotation model.
This is a good attempt that goes in the right direction, I'll try to find some time to rework this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure - any insights & examples are appreciated too.

hujambo-dunia and others added 5 commits October 22, 2024 13:23
Co-authored-by: Ahmed Hamid Awan <qe66653@umbc.edu>
…kflows Landing Page (instead of design consistency with Workflows Annotation History Panel). Added 'show-actions' Boolean parameter to focus user on running the workflow as consistent with their intended behavior from the prior step; when 'show-actions' equals False, all actions are hidden (note: not same terminology as technical 'read-only' as 'show-actions' also prohibits Share/Favorite which are separate from being able to Edit a workflow).
@hujambo-dunia
Copy link
Contributor Author

hujambo-dunia commented Oct 23, 2024

Ahmed: Minor change requests

  1. Backend change in managers/workflow.py: workflow_id: trans.app.security.encode_id(workflow.id) COMPLETE
  2. Create new frontend component based on following but only pull in last 4/right-most actions: WorkflowInvocationHeader.vue
  3. After expandable header, add 2nd secondary component for README.md (check out 3rd party dependency)

hujambo-dunia and others added 3 commits October 25, 2024 14:37
Co-authored-by: Marius van den Beek <m.vandenbeek@gmail.com>
Co-authored-by: Marius van den Beek <m.vandenbeek@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants