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

bring grids for (published) pages on par with workflows #16209

Merged
merged 29 commits into from
Jun 21, 2023

Conversation

martenson
Copy link
Member

@martenson martenson commented Jun 7, 2023

this is waiting on #16163, will stay draft until then

  • unifies the component for published/personal pages grid
  • reuses components from workflows grids
  • converts some components to TS/compo, brings others closer to current standard
  • brings new junit, API, and selenium test coverage

personal grid

  • before Galaxy
  • after Galaxy___martenson

published grid

  • before Galaxy
  • after Galaxy___martenson

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.

@github-actions github-actions bot added this to the 23.1 milestone Jun 7, 2023
@martenson martenson marked this pull request as draft June 7, 2023 21:55
@jmchilton
Copy link
Member

Awesome - thanks so much!

@martenson martenson mentioned this pull request Jun 8, 2023
22 tasks
@martenson martenson marked this pull request as ready for review June 12, 2023 19:05
lib/galaxy/schema/schema.py Outdated Show resolved Hide resolved
Copy link
Member

@jdavcs jdavcs left a comment

Choose a reason for hiding this comment

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

backend looks good!

@itisAliRH itisAliRH self-requested a review June 13, 2023 14:32
Copy link
Member

@itisAliRH itisAliRH left a comment

Choose a reason for hiding this comment

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

Overall, looks great to me👍
I've reviewed the front-end changes and left some comments.

client/src/components/Page/PageDropdown.vue Outdated Show resolved Hide resolved
client/src/components/Page/PageIndexActions.vue Outdated Show resolved Hide resolved
client/src/components/Page/PageList.vue Outdated Show resolved Hide resolved
client/src/components/Page/PageList.vue Outdated Show resolved Hide resolved
@jdavcs
Copy link
Member

jdavcs commented Jun 14, 2023

API test failures (due to deadlock) on first run were unrelated.

@martenson
Copy link
Member Author

the whole lib/galaxy_test/selenium/test_pages.py::TestPages passes fine locally, not sure what is up with it here

@jdavcs
Copy link
Member

jdavcs commented Jun 14, 2023

the whole lib/galaxy_test/selenium/test_pages.py::TestPages passes fine locally, not sure what is up with it here

This one fails for me locally. The test_histories_published.py test passes, so that one is unrelated.

@martenson
Copy link
Member Author

setting values in the toolform fail even with d057da2

screenshots show just masthead rendered, browser log is empty, driver log is empty

locally this works fine even with headless=0 and me watching it

I'll look into it more tomorrow

- drop the relevant jest tests since b-modal attaches outside of the wrapper and
is not findable to my knowledge
- could be likely replaced by a selenium test?
@martenson
Copy link
Member Author

martenson commented Jun 16, 2023

at this point I really don't know what's up with this test; I cannot reproduce it locally and the debug info from CI does not give me a clue

any insights are very welcome

@martenson
Copy link
Member Author

I don't know why but the last CI run ran on an outdated code, referring the page_open_with_name method that has been renamed and is not present in the codebase anymore

(https://github.com/galaxyproject/galaxy/actions/runs/5294667001/jobs/9584250356?pr=16209#step:10:23)

@davelopez
Copy link
Contributor

at this point I really don't know what's up with this test; I cannot reproduce it locally and the debug info from CI does not give me a clue

When the Selenium tests pass locally but fail consistently in CI I always try to set an URL prefix like GALAXY_CONFIG_GALAXY_URL_PREFIX=/galaxypf, I had the same problem once and Marius pointed me to this, then I can get the same results as in CI as it uses the prefix for all tests.

Also, when I get an empty page just with the masthead showing (since I now know it is a prefix-related issue), I look carefully at the URL and I usually find the reason :)

image

The prefix got duplicated.

@jdavcs jdavcs merged commit e12c799 into galaxyproject:dev Jun 21, 2023
@jdavcs
Copy link
Member

jdavcs commented Jun 21, 2023

Thank you, @martenson!

@martenson martenson deleted the more-pages branch June 21, 2023 20:33
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.

5 participants