Assessment tool group tabs and enhanced functionality

[jarmoza]

• Assessment tool groups are now placed in the Vuex store and used to generate separate annotation tabs on the annotation page.
• All columns categorized as an 'Assessment Tool' must now be grouped in order to proceed to the annotation page, and will be found within their corresponding tool group tab on the annotation page.
• Tool group name and column (tool) list are fully editable as well as removable. In order to begin modifying a tool group, select its row in the tool group table. The tool group's current name as well as all available tools for grouping (its current tools and other un-grouped tools) will show up in the same interface where tool group creation happens. Button text will change to '~ Modify Tool Group'
• Uncategorizing columns in the column linking table will remove them from currently saved tool groups. If all columns in a tool group are uncategorized in this way, the group itself is deleted
• Changes made in the tool group interface on the categorization page are reflected on the annotation page (group name and tools per group).

This PR addresses functionality listed in both #118 and #138. See each issue for the desired functionality checklists.

[surchs]

Hi @jarmoza. Thank you for the PR. This looks like a lot of work. Could you please do either:

• merge the main branch into this one OR
• rebase this branch onto the main branch (preferred)

so that we we can look at this together with the other recent additions and fixes (and also resolve these pesky merge conflict messages).

[surchs]

The Object.prototype... thing made me look twice. If I understand right, ESlint flags this because the default this.details.hasOwnProperty could fail if this.details happens to have a property called "hasOwnProperty"? The recommended way to work around this (if legacy browser support isn't a barrier) seems to be to use hasOwn instead, which is the replacement of hasOwnProperty.

I'd suggest we use hasOwn here or if not add a comment why were are using this less expected form.

[jarmoza]

Yup. That's right. Object.prototype.hasOwnProperty.call is the suggestion to make sure of the correct behavior. hasOwn is newer, considers a few more edge cases, and "is more intuitive." Sounds right.

[surchs]

Thanks for the PR @jarmoza, this is working and looks very nice!
It also solves our need to group columns together so we can annotate them together and then define missing values.

That said, I think in a future PR we should redesign the tool-grouping workflow. The main issue is that we now have an additional thing in the store to keep aligned with the columnToCategoryMapper we already have, and this alignment is hard to do, we need watchers, and refresh methods and other things that are easy to break. See for example the bug I described where, if you assign two columns to a tool, and then remove one of the columns via the annotation window, the column is still assigned to the tool, but no longer designated as a "assessment tool" category.

I think at the core, the "map columns to tool groups" workflow is the same as the "map columns to categories" workflow. The only difference is that the category you are assigning columns to (i.e. the tool group) has to be created first. That is, if we find a way of creating new categories for the assessment tool groups, then we don't have to have separate store variables and UI elements to keep columns and tool groups aligned with columns and categories.

That said, the functionality right now is all we really need to start using the tool (and to add the assessment tool group annotation functionality) and so I suggest we merge this PR with some minor changes (as commented) and think about my above points in a separate issue.

[surchs]

Is this change related to the annotation tabs? If so, I don't think I understand what it does. Otherwise I'd say we add that to the missing values PR and discuss it there.

[jarmoza]

I am too a bit puzzled by this line. I don't quite recall writing it. Just looking at what the function is supposed to be doing though, perhaps an answer to this question would clear things up.

When a value is removed from the missing value table how else is tableItems being updated? I don't see any other place it's touched other than in created().

[surchs]

neat!

[surchs]

There is something weird going on here when the columnToCategoryMap have been changed (e.g. by removing a column from a tool group in the annotation window). The problem seems to be that although this.assessmentToolGroups clearly has a property items with a non-empty array, directly accessing this array via this.assessmentToolGroups.items (e.g. console.log(this.assessmentToolGroups.items) shows an empty array.

Take a look here:

If you access the variable directly or watch it via vue-dev tools, all looks normal. But obviously you cannot iterate over an empty array so you never get to check if anything has to be removed here.

As I mention in the review, i think the real solution to this is to redesign the way the tool groups are implemented. But we should do this in a later PR, right now we get what we need and this bug is something we can work around.

[jarmoza]

1. Yes, to redesign. One of the reasons I had wanted to take a look at a potential object-oriented reworking is because things are beginning to feel to disparate/unwieldy in the store and in turn too constrictive (a.k.a. 'brittle'). Too many sources of truth. That said, that's not for now. So let's discuss this smaller internals redesign next.
2. I'm going to take a look into this for a little bit so I can better understand what is happening.

[jarmoza]

So, from what I can tell there is some disconnect in how you're viewing the object via the browser (client) console. Debug messages from the server as well as Vue dev-tools reveal assessmentToolGroups.items to be correctly populated.

That said, I have produced a fix for the issue of the table not being updated and though we should readdress how this is done in the redesign we mentioned, we have some nice "magic" at hand.

In the annotation pages unlinkColumnFromCategory method - which is called when a column is removed from the annot-column component, I have added the following third step to appropriately update the tool groups in the store.

                // 3. If this column was a grouped tool, remove it from its group
                if ( this.isToolGrouped(p_event.removedColumn) ) {

                    // A. Remove the tool from its group
                    const groupName = this.getGroupOfTool(p_event.removedColumn);
                    this.$store.dispatch("removeToolFromGroup", {
                        group: groupName,
                        tool: p_event.removedColumn
                    });

                    // B. If its former group is now empty of tools, remove the group itself
                    for ( const groupName in this.toolGroups ) {
                        if ( 0 === this.toolGroups[groupName].length ) {

                            this.$store.dispatch("removeToolGroup", { name: groupName });
                        }
                    }
                }

This duplicates behavior from the categ-tool-group component itself, but applies the changes we want reflected on the categorization page.

And here's the Vue "magic" part. If every tool is removed from its tab on the annotation page (annot-columns component), the tab itself is now removed.