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

[ENH] Restore download functionality for refactor/new meta getter #433

Merged

Conversation

jarmoza
Copy link
Contributor

@jarmoza jarmoza commented May 29, 2023

Closes #427
Closes #416
Fixes #172

Changes proposed in this pull request:

  • Refactor of download page to use new meta getter getJsonOutput
  • Adds new field and getter for dataDictionary.filename to (re)use for the new output json file
  • Download page 'e2e' test checks if a file has has downloaded after button click (solution suggestion found here: https://stackoverflow.com/questions/73971115/how-do-i-list-the-contents-of-the-download-directory-in-cypress-i-need-to-make)
  • Adds app state loading code for download page
  • Update unit test for meta getter getJsonOutput with mocks for its getters. This overcomes an issue where calling a getter from a getter worked one way in e2e/manual testing but not in unit testing and vice versa.

@jarmoza jarmoza marked this pull request as draft May 29, 2023 18:07
@jarmoza jarmoza added download page data store maint:coverage Test coverage improvements that were not included in feature prioritization maint:refactor Simplifying or restructuring existing code or documentation. labels May 29, 2023
@jarmoza jarmoza marked this pull request as ready for review May 29, 2023 18:16
@jarmoza jarmoza changed the title [WIP] Restore download functionality [MAINT] Restore download functionality May 29, 2023
@jarmoza jarmoza changed the title [MAINT] Restore download functionality [MAINT] Restore download functionality for refactor/new meta getter May 29, 2023
@jarmoza jarmoza linked an issue May 29, 2023 that may be closed by this pull request
3 tasks
@jarmoza jarmoza linked an issue May 29, 2023 that may be closed by this pull request
3 tasks
@surchs surchs self-requested a review May 29, 2023 19:12
@jarmoza jarmoza changed the title [MAINT] Restore download functionality for refactor/new meta getter [ENH] Restore download functionality for refactor/new meta getter May 29, 2023
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Very exciting milestone @jarmoza 🎉. We definitely need to celebrate this big achievement with everyone, especially @rmanaem!

The PR looks good, my comments are mostly 🍒 or thoughts. Take a look and decide, then good to squash merge 🧑‍🍳

Once you do, please directly open the next PR to merge dev_... into master! We can deal with any required rebasing in that other PR then.

cypress.config.js Show resolved Hide resolved
pages/download.vue Show resolved Hide resolved
pages/download.vue Show resolved Hide resolved
store/index.js Show resolved Hide resolved
store/index.js Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!
One thing I'm noticing here is that because all of the getters have to be stubbed, I'm not sure what we'll do once we encounter some weird edge cases that we'll also want to test. If every one of these needs it's own setup and each setup will be as long as this one, tests may get a bit hard to read.

But for now this reads well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may be beyond an aesthetic issue actually. Something I've noticed is that debugging the getters if they are plugged in via the import from index.js is not available via console-style debugging. And in fact, it seems that beforeEach is working in a different or at least at a different time to properly setup the variables for each test. I've noticed there's a difference between assigning getters to store.state.getters in beforeEach vs. inside of a test. This is something that should be documented in our Cypress wiki as well.

cypress/support/commands.js Show resolved Hide resolved
cypress/e2e/page/download-pagetests.cy.js Show resolved Hide resolved
cypress/e2e/page/download-pagetests.cy.js Show resolved Hide resolved
@jarmoza jarmoza merged commit 4b585c6 into dev_components_talk_to_store_directly May 31, 2023
@jarmoza jarmoza deleted the restore-download-functionality branch May 31, 2023 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data store download page maint:coverage Test coverage improvements that were not included in feature prioritization maint:refactor Simplifying or restructuring existing code or documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restoration of download functionality for refactor Mutation to store the data dictionary filename
2 participants