Column linking table refactor + component test #230

jarmoza · 2022-11-17T21:38:53Z

Refactor changes

All props but selected category eliminated. Uses mapState instead
Table column labels and column to category table are no longer props

The labels are defined in the component's data and are aligned with other components using uiText object
The creation of the column to category table is done when the component is created (see setupColumnToCategoryTable - which has been moved from the categorization page)

Component tests

Tests two scenarios to make sure the correct data is being emitted by click on the table rows:

Linking a category to a column
Unlinking a column from a category

Closes #205

surchs

Hey @jarmoza, thanks a lot for the PR 🎉 . Again, really good size of change, very nice to review!

I can confirm that the changes run and pass locally (I guess we already know this now that the CI is working). I do have three main comments (details in inline replies):

I think there is logic in the component that could be refactored into a state getter. For example, you're assuming that a number of store.state variables exist, access them directly, and then pick the data you need from across them. That's a lot of work, I think a getter could do the heavy lifting here (e.g. get you the columns, and a second to get you the descriptions). We have other components who need similar things, and so we can start reusing getters.
In some places the data flow isn't completely clear to me. There are actions called in response to click-events, but the component figures out what action to call given the current store state (I think that'd be better handled by the store itself), and then a second emit is done directly to the parent. Maybe that'd be something we can briefly chat about in case I don't get it.
Like in Refactor of category-select-table component #229, I think we should really have less information about specifics in the store / store data in the tests. So ideally we could just hard-code the data that the getters are supposed to provide to the component during testing, and then not have to model the entire store state that is currently mostly used to fill the getters.

surchs · 2022-11-21T18:02:41Z

components/column-linking-table.vue

            applyCategory(p_row, p_index, p_event) {

-                // Tell the parent page that a column has been linked with a category
-                this.$emit("column-name-selected", { column: p_row.column });
+                const dataForStore = { column: p_row.column };


If I understand correctly here: when I click on a column, this applyCategory gets called. And it then tells the store: "hey, someone clicked on this column while this category was active - can you go figure this out?". So I think we can leave it up to the store to figure out whether the meaning of this click is going to be a "unlink" or a "link" action. That is, I think here we should just call the store mutation/action

applyCategory now calls the action (to be implemented in store refactor) alterColumnCategoryRelation. This will take a payload of the column and category and determine whether the column should be linked with this category (if it has no category, or a different category associated with it) or if it should be unlinked (the input category is the same as the one it is currently associated with).

surchs · 2022-11-21T18:04:10Z

components/column-linking-table.vue

        },

        methods: {

+            ...mapActions([


any reason these need to be actions instead of mutations? are you planning on using actions for any multi-mutation event? Or do you expect there to be some asynchronous logic to happen here?

These will become mutations in the next store refactor, and I will be creating a new, general store refactor issue to list these changes as these come in.

A new action has been added (alterColumnCategoryRelation) in 01377f2

The idea here is that this action will contain the logic that decides whether one of the two methods should be called given the input (column/category) and the state of the columnTocCategoryMap in the store.

surchs · 2022-11-21T18:05:00Z

components/column-linking-table.vue

+                }
+
+                // 2. Tell the categorization page a column has been clicked
+                this.$emit("column-name-clicked", dataForStore);


Why do we need this emit here? At this point I have already told the store about the column click. Could whoever is listening to this emit not receive that information from the store directly?

As discussed in our huddle, the current function of this emit is to let the parent page know to check if the next page (annotation page) is ready to be accessed given the state of the store.

I am leaving in the emit for now as I would like to handle the general move of next page access to the layout/store in a separate PR.

surchs · 2022-11-21T18:08:37Z

components/column-linking-table.vue

+                this.$emit("column-name-clicked", dataForStore);
+            },
+
+            setupColumnToCategoryTable() {


I get what this method does. But given how many assumptions we're making here about how the store will be organized, I think it might be cleaner to just ask the store to provide the data elements we want here directly - like we're doing in other components. E.g. one getter to get the column names of the data table, and a second getter to get the description for each column if there is one.

Then we don't have to do so much work in the component and also have an easier time if we decide to change the specifics in the store.

As per our discussion of refactoring/writing tests toward an ideal store, this function is removed. Its logic will be split apart/reworked and added to the store. (This work will be added as a subtask of the new store refactor issue I will be creating.)

Instead, now the table rows are computed property that returns an array of objects with keys for column name, column description, and category. The first two pull from a to-be-implemented columns store getter that returns an array of objects containing a column name and description per array entry.

The unused column of category has also been added to the table (and is now utilized) as another visual indicator of the link between column and category. See data().uiText.tableFields.

surchs · 2022-11-21T18:09:53Z

cypress/component/column-linking-table.cy.js

+
+    actions: {
+
+        dispatch: (p_actionName, p_payload) => { }


I really like this. I haven't added the actions to my tests but I think it helps to document them explicitly. One thing though: shouldn't there be two actions here, linkColumnWithCategory and unlinkColumnFromCategory?

linkColumnWithCategory and unlinkColumnFromCategory will be converted to mutations and a new action alterColumnCategoryRelation has been added that will call (in future store refactor) either depending on the given action input and state of the store field columnToCategoryMap.

surchs · 2022-11-21T18:10:18Z

cypress/component/column-linking-table.cy.js

+    getters: {}
+};
+
+store.getters = {


why are the getters from above overwritten here?

The mock store and getters have been completely reworked as per our discussion in Slack huddle.

See 01377f2

surchs · 2022-11-21T18:11:01Z

cypress/component/column-linking-table.cy.js

+                $store: store.actions
+            },
+
+            computed: store.getters,


I don't think the getters are used by the component at all. Is this an oversight here?

The mock store and getters have been completely reworked as per our discussion in Slack huddle.

See 01377f2

jarmoza · 2022-11-22T21:05:31Z

@surchs Take a look, but I think 01377f2 addresses all of the changes we discussed yesterday.

surchs

Thanks @jarmoza, I think this looks great. One question: did you mean to keep the emit in there? I may have misunderstood but I thought the record of the categorization action would be recorded in the store because of the dispatched action. Could you take a look if you still need that emit (and the test for it)?

Otherwise, good to go.

surchs · 2022-11-23T01:17:22Z

components/column-linking-table.vue

+                this.alterColumnCategoryRelation(payload);
+
+                // 2. Tell the categorization page a column has been clicked
+                this.$emit("column-name-clicked", payload);


Did you still want to remove this emit here, now that the action will handle things?

@surchs I was thinking of leaving the emit in place and removing it when I do the changeover for next page accessibility with the layout/store. (See my earlier comment.)

I suppose it doesn’t matter when the removal happens, but I thought removing it in this PR would be mixing issues.

Ah OK, I see the point. My view was that we should refactor these components without worrying a lot about what other app parts still depend on this. So if you remove the emit now, the the categorization page will be broken. But we have to refactor the categorization page anyway and I think having a clean component to do this against might be easier than having to remember that this emit should be removed. But your call. Maybe a NOTE is enough.

I was torn over it, and see the logic of both sides. Let's break things. I'll at least leave a NOTE in its place with a final commit and then merge.

jarmoza added 5 commits November 16, 2022 17:19

Streamline IO for data dict refactor

3cb4d01

Component test for linking table - almost working

f454e76

Explicit mocks + working simple link test

78d59cb

Remove debug messages

9b1676f

Fix for first test and addition of second.

893f2c0

jarmoza added categorization page refactor labels Nov 17, 2022

jarmoza requested a review from surchs November 17, 2022 21:38

jarmoza closed this Nov 17, 2022

jarmoza reopened this Nov 17, 2022

jarmoza changed the base branch from master to dev_components_talk_to_store_directly November 17, 2022 21:45

jarmoza marked this pull request as draft November 17, 2022 22:11

jarmoza added 2 commits November 18, 2022 14:37

Component now works with store directly

609a3a5

Now checking for store link/unlink dispatch

1c79ec0

jarmoza marked this pull request as ready for review November 18, 2022 19:38

surchs requested changes Nov 21, 2022

View reviewed changes

PR changes for simplifying component and test

01377f2

surchs approved these changes Nov 23, 2022

View reviewed changes

Remove emit to categpage for store/layout refactor

f76e1da

jarmoza merged commit 644347d into dev_components_talk_to_store_directly Nov 23, 2022

jarmoza deleted the column-linking-datadict-refactor branch November 23, 2022 16:22

jarmoza mentioned this pull request Nov 23, 2022

Store Refactor (Draft) #231

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Column linking table refactor + component test #230

Column linking table refactor + component test #230

jarmoza commented Nov 17, 2022 •

edited by surchs

Loading

surchs left a comment

surchs Nov 21, 2022

jarmoza Nov 22, 2022 •

edited

Loading

surchs Nov 21, 2022

jarmoza Nov 22, 2022

surchs Nov 21, 2022

jarmoza Nov 22, 2022

surchs Nov 21, 2022

jarmoza Nov 22, 2022

surchs Nov 21, 2022

jarmoza Nov 22, 2022

surchs Nov 21, 2022

jarmoza Nov 22, 2022

surchs Nov 21, 2022

jarmoza Nov 22, 2022

jarmoza commented Nov 22, 2022

surchs left a comment

surchs Nov 23, 2022

jarmoza Nov 23, 2022 •

edited

Loading

surchs Nov 23, 2022

jarmoza Nov 23, 2022

Column linking table refactor + component test #230

Column linking table refactor + component test #230

Conversation

jarmoza commented Nov 17, 2022 • edited by surchs Loading

surchs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarmoza Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarmoza commented Nov 22, 2022

surchs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarmoza Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarmoza commented Nov 17, 2022 •

edited by surchs

Loading

jarmoza Nov 22, 2022 •

edited

Loading

jarmoza Nov 23, 2022 •

edited

Loading