-
Notifications
You must be signed in to change notification settings - Fork 6
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
createColumnToCategoryMap mutation + required getter + test #291
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @jarmoza. Pretty cool how much stuff can be in 60 changed lines - this one is pretty tricky.
The main thing is that you can't access getters
from inside mutations
. I wasn't aware of this either before I tried to understand why your test looked the way it did (yay for testing 🎉). But because of the way things are, we need to make a decision on how we want to implement this. Take a look at my inline comments, and then we should probably get on a call with @rmanaem to make a decision.
As a sidenote: you have addressed both #259 and #255. We should discuss how we handle these situations where we discover an overlapping dependency after we have already assigned the issues so we don't duplicate the work.
getColumnNames(p_state) { | ||
|
||
// Returns list of columns from the loaded data table | ||
return ( 0 === p_state.dataTable.length ) ? [] : Object.keys(p_state.dataTable[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any valid path for this getter to be called before a dataTable
has been loaded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessarily a straightforward question to answer at the moment because our refactor of the pages for the annotation tool is still in progress. The most direct answer is that it depends on how a getter such as this is used and where – and how gracefully we want to handle failures if the components we hand invalid values to do not.
If a user loads something like the categorization page directly – from a previous browser session for instance and Vue server is still up and running but with a wiped Vuex store – a check like this offers a more graceful failure than what would occur if an empty dataTable
's 0th index is accessed.
With the conditional check, the empty array would be offered to the column-linking-table
component's Bootstrap table. Otherwise the table would be handed undefined
(or the page itself would crash).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, that's a really good point. So there is at least one way we know how this getter could be called before a dataTable has been loaded (the one you described). In that case my follow-up question is: shouldn't we then also test in the components that they are in fact OK with receiving these empty arrays? Not for this PR. But if we say: you can reload the page and we'll make sure that the page doesn't crash and instead give you a valid but empty UI, then we should test that this is in fact true.
But for now, agreed that the check makes sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we then also test in the components that they are in fact OK with receiving these empty arrays? Not for this PR. But if we say: you can reload the page and we'll make sure that the page doesn't crash and instead give you a valid but empty UI, then we should test that this is in fact true.
Absolutely true. Relevant component tests should test for invalid input if such a scenario exists. And I would say this would extend to page tests, so we can give our users some helpful feedback as to why something has gone wrong and how they might fix it.
@surchs I think this is ready to go with my latest commit. However, I have changes for For your task that latter call will be need to be moved to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @jarmoza. I left one more comment on deep.equal
vs equal
. Take a look, but I think we can also discuss that some other time. Either way, good to go!
This PR addresses #259, the creation of a mutation that creates the column to category map.
@surchs There are a few new complexities with the introduction of this mutation for our process of adding each new element for the store refactor.
Going through the subtasks in #259:
The trouble here is that the index page requires
createColumnToCategoryMap
. This requires a check with the index page's old e2e test which is currently broken due to page access (nav item for categorization does not start out as disabled).Unit tests have been created for the
createColumnToCategoryMap
mutation and also the required, new underlying gettergetColumnNames
dataTable
has been added toindex-refactor
as it is required by thecolumnToCategoryMapping
. (This name should be adjusted when we get a chance minus the -ping).The addition of the getter
getColumnNames
is the first time we have required a getter in addition to a state field. The issue is what gets passed to the new mutation/getter in question (in this case,createColumnToCategoryMap
). Since there is some function mapping magic that occurs in the actual Vuex store, callingp_state.<getterName>
is possible. However, in using our current store mock it is not.