Phase 2 Store Refactor - Integration Part 1 - Annotation page access, tabs refactor, column-category unlinking

[jarmoza]

In an effort to keep PRs small, this PR closes the first step of restoring the annotation page (access + tabs) now that the second phase of our store refactor is (mostly) complete. As such this PR is not yet testable via our e2e page tests since the annotation page itself is functionally incomplete and its related e2e tests need to also be refactored to adapt to changes we have made. (Closes #348.)

In addition to basic access to the page with its primary components loading, this PR also addresses the following, as noted in #347 :

• Tabs now must look at new store configuration for info on their construction (previously in store field annotationDetails)

Here is a file by file explanation of the changes made thus far:

annot-categorical.vue

• v-select value attribute is temporarily disabled as this reflects a getter we missed in our refactor discussions: getSelectedOption. @surchs also noticed this and there will be a brief discussion about this in Refactor: handle controlled terms as objects #374
• Refactors for new parameterization of changeMissingStatus and selectCategoricalOption getters

annot-categorical.cy.js

• Tests updated for refactor and brought in line with how annot-continuous-values is tested (re: act/assert calls)

annot-continuous-values.vue

• Call to setHeuristic mutation brought in line with how annot-categorical handles v-select input by calling the relevant mutation directly. Removed wrapper function commitHeuristic

annot-continuous-values.cy.js

• Updated changeMissingStatus mock to take object argument its real store version takes

annot-tab.vue

• Simple removal of now unused id-field

annotation.vue

• Annotation tab creation no longer uses removed annotationDetails store field but rather columnToCategoryMapping (and colorInfo for coloring)
• The annot-tab child component's props have been greatly reduced here. Keeping commented out version for now for any remaining functionality needed in the next integration PR; specifically for missing values.
• A categorySkipList field has been added to the page for generality. Really, our current hardcoded behavior is that no tab should be created for Subject ID and this is passed to the new getMappedCategories getter
• Keeping commented out store maps and functionality until next integration PR
• See index.js below for new criteria created for access to the annotation page

index.js

• New getter getMappedCategories that looks at columnToCategoryMapping and returns a unique list of categories that have thus far been linked to columns in the input data table. A optional list of categories to skip returning is given as a parameter. The existence of this getter is up for discussion, but its creation seemed to make sense as opposed to placing this store functionality inside annotation.vue
• Because we have not yet had a discussion on how we want optional data dictionary key value pairs to be created, it is currently necessary to check for the existence of the missingValues list in both the getMappedColumns getter and changeMissingStatus mutation.
• New isPageAccessible criterion has been added for moving to the annotation page: notOnlySubjectIDCategorized. Discussion point here as well is whether or not we want to allow users to move to the annotation page without categorizing a column other than one as 'Subject ID'. This doesn't make sense if we have no other categorizations as with the current implementation no tabs would exist on the annotation page. In the previous implementation, all categories had hardcoded tabs via annotationDetails
• Change selectCategoricalOption mutation's parameters to one parameter object to bring it line with the rest of our store mutations
• Fix for missing values check in getUniqueValues

store-getter-getMappedCategories.cy.js

• New unit tests for new getter getMappedCategories for annotation page

store-getter-isPageAccessible.cy.js

• Changed test to account for new criteria where at least one column must be linked to a non-'Subject ID' category for the user to proceed to the annotation page

store-mutation-selectCategoricalOption.cy.js

• Updates for new object argument for selectCategoricalOption mutation

[jarmoza]

Okay @rmanaem, test fixes are up. Ready to review.

[jarmoza]

Okay @rmanaem. Seb's PR is merged in and tests are green.

[rmanaem]

Thanks for the well written PR description @jarmoza!
I added some in-line comments. Aside from that I was wondering why you decided to change component tests to use beforeEach.

[jarmoza]

@rmanaem

Thanks for the well written PR description @jarmoza!

You're welcome! I thought it would be helpful given the amount of changes across files.

I added some in-line comments. Aside from that I was wondering why you decided to change component tests to use beforeEach?

That is an excellent and relevant question. I mentioned this to Seb yesterday. I have noticed that when Cypress begins a new individual test that the functions defined in store.getters in particular were being read as "undefined." My workaround for this was to reinitialize the store in beforeEach before each individual test. JSON.stringify does not correctly output values assigned to an object key if they are function definitions, making the issue more difficult to debug. Not happy that I don't understand why this is occurring, but there is at least this beforeEach workaround which is not the worst thing in the world to have to do.

[rmanaem]

You're welcome! I thought it would be helpful given the amount of changes across files.

It really is!

have noticed that when Cyoress begins a new individual test that the functions defined in store.getters in particular were being read as "undefined." My workaround for this was to reinitialize the store in beforeEach before each individual test. JSON.stringify does not correctly output values assigned to an object key if they are function definitions, making the issue more difficult to debug. Not happy that I don't understand why this is occurring, but there is at least this beforeEach workaround which is not the worst thing in the world to have to do.

Yea, I think we talked about this too but this only affects the unit test, not component tests.

[jarmoza]

@rmanaem

Yea, I think we talked about this too but this only affects the unit test, not component tests.

This is in reference to annot-categorical.cy.js in particular? I just reverted to defining the store object above the describe once and it works. Shall I commit the change?

[rmanaem]

This is in reference to annot-categorical.cy.js in particular?

I was referring to component tests in general. I was curious why you decided to use this syntax instead of the old one.

I just reverted to defining the store object above the describe once and it works. Shall I commit the change?

I leave that up to you but other than that this PR is good to go! 👨🏼‍🍳

[jarmoza]

@rmanaem I think let's save that for another PR. I'll make an issue to look over the tests for consistency in setup.