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

Workflow landing improvements #18979

Merged
merged 10 commits into from
Oct 22, 2024
Merged

Workflow landing improvements #18979

merged 10 commits into from
Oct 22, 2024

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Oct 10, 2024 •
edited
Loading

Optimizing the BRC / IWC case first here:

  • Separate creation of public and private landing requests
  • Don't claim the landing page if public=true is passed in request
  • Allow passing in a TRS url so we don't need to hardcode instance-specific workflow ids
  • Don't reimport TRS workflow if the user already has it
  • Redirect user to login / registration form if not logged in, then send them back to landing page

Also adds a selenium test that exercises public and private landing requests.

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.

@mvdbeek mvdbeek marked this pull request as draft October 10, 2024 15:54
@github-actions github-actions bot added this to the 24.2 milestone Oct 10, 2024
@jmchilton jmchilton mentioned this pull request Oct 13, 2024
Merged
2 tasks
@mvdbeek mvdbeek force-pushed the workflow_landing_improvements branch 8 times, most recently from 85f873f to aa20274 Compare October 20, 2024 22:25
@mvdbeek
Separate private and public landing requests
4f497a3 
Private requests must be claimed before use. Public / Anonymous requests
only depend on the public flag, so anoymous users can create private
landing requests and authenticated users can create public requests.

Tests should cover all of these scenarios.
@mvdbeek
Fix unit tests
f4a779e
@mvdbeek
Parameterize public and secret props in workflow landing component
9608202 
Chose to make the url query param `client_secret` to match the API
parameter.
@mvdbeek
Add selenium tests for workflow landing
d5879db
@mvdbeek
Fix test_stock.py unit test
2565bb7 
We'd treat macros.xml and sample_tool_conf.xml as potential tools and
fail `assert tool_source.parse_id()`.
@mvdbeek mvdbeek force-pushed the workflow_landing_improvements branch from aa20274 to 2565bb7 Compare October 20, 2024 22:35
@mvdbeek mvdbeek marked this pull request as ready for review October 20, 2024 22:37
@mvdbeek mvdbeek requested a review from jmchilton October 20, 2024 22:40
jmchilton
jmchilton reviewed Oct 22, 2024
View reviewed changes
client/src/components/Workflow/Run/services.js
export async function getRunData(workflowId, version = null) {
let url = `${getAppRoot()}api/workflows/${workflowId}/download?style=run`;
export async function getRunData(workflowId, version = null, instance = false) {
let url = `${getAppRoot()}api/workflows/${workflowId}/download?style=run&instance=${instance}`;
Copy link
Member

Choose a reason for hiding this comment

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

Fetching by instance ID gave me really unexpected behavior in workflow rerun API - I'd check that out and make sure this is giving you the expected behavior and that you and I are the on the same page about what that expected behavior should be in this case. The PR I'm referring to is #18985 and the commit that fixed the API behavior IMO is cc8f1b6 - you may need to apply the same fix to this endpoint if we're on the same page.

@jmchilton
Copy link
Member

Thanks for making those changes - I really appreciate it. The API makes a lot of coherent sense to me now but I hate creating extra work for you. Also I love seeing Selenium tests - thanks so much for doing that!

@jmchilton
Copy link
Member

I never see those tests fail unless I have broken something but there isn't logging, it won't let me rerun them, and I have no clue how any of those changes could break those tests - you haven't touched tool parsing code here - so I am going to assume those spurious failures I just haven't seen before.

@jmchilton jmchilton merged commit 639d302 into galaxyproject:dev Oct 22, 2024
56 of 57 checks passed
@github-actions GitHub Actions
Copy link

This PR was merged without a "kind/" label, please correct.

@nsoranzo nsoranzo deleted the workflow_landing_improvements branch October 22, 2024 13:06
@mvdbeek mvdbeek mentioned this pull request Nov 28, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmchilton jmchilton jmchilton left review comments

Assignees
No one assigned
Labels
area/API area/database Galaxy's database or data access layer area/testing area/tool-framework area/workflows kind/enhancement
Projects
None yet
Milestone
24.2
Development

Successfully merging this pull request may close these issues.
3 participants
@mvdbeek @jmchilton @nsoranzo