Actually, just an update to my first item on Shopping Charts:
It looks like you are actually handling that if you are inside a Concept Set in the Criteria Feature's concept set, and then you navigate over to the vocabulary search, the shopping carts DO modify the concept set that you have open in the criteria feature.

But, there's some confusion here: if i have an open Criteria Feature concept set, and then I open up a Concept Set from the repository, toggling the shopoping charts does modify the last thing that I opened, but there are also shopping carts that are 'lit up' from the original concept set that was opened. So there is some confusion in the UI about which concepts belong to which concept sets.

It is this confusion that I originally said that the 'shopping cart' design should be thrown out and we should instead have context-local concept set vocabulary searching. As you can see from the complexity of the code, every time we have a new source of concept sets, we have to go back to the main app.js and define a new set of IF statemets about which concept set from which 'nav' we're dealing with, and sometimes you can have multiple concept sets in a single nav (such as a concept set under CC and a concept set under FE). I think it's great what you've managed to get working this far, but the architecture of the vocabulary search + concept set editor coupling leads to some very challenging problems when you try to make it work for any concept set from any Atlas asset.

@chrisknoll Thank you for such deep review and detailed explanation. It's really helpful.
Made the Concept Set tab visible only for Feature Analyses with criteria.
Fixed issue with custom FA design page.
Continue working on the dirty flag and new architecture for the context-local searching

Thanks @wivern. I know this is complicated, but maybe there's a simple solution:

What if each nav-module maintained their own activeConceptset in atlas-state such that in our search UI, we could provide a dropdown of activeConceptsets that would allow the user to choose which concept set they would like to manage. Selecting a different concept set will 'refresh' the highlighted shopping carts to show which concept sets are in the selectedConceptset. Closing the individual assets in the nav-modules (like CC or Cohort Defintion) will remove that activeConceptset, and thus not appear in the activeConceptSets in the search UI.

This is a very high-level idea and may have some cases where I didn't think of, but I am trying to think of a way forward that wouldn't require you to rewrite the entire search UI and instead just make the searchUI aware of the conceptSets that could be edited based on the concept sets that were opened in the nav-module.

Any updates on this PR?

Any updates on this PR?

@chrisknoll Unfortunately no. Thank you for review and suggestions. It's really helpful.
Hope I would be able to implement as soon as possible.

Ok, Thanks for the update. Please let me know if you would like any assistance on this.

I am looking at this, but this PR also is paired with a PR related to WebAPI, and I have some open questions on the code changes over there. So, I will be working on those 2 PRs in parallel.

@chrisknoll Hello! Thanks for reply. I think we should also wait until some of other opened PRs in Atlas will be merged and then i'll update this one again according to the latest master because this PR affects many things.

Hi, @vlbe. I'm under a bit of time constraint and prioritization, and I'd really like to focus on getting these 2 prs done. In my mind, it woudl be easier to get this large chunk of code updated, and update the other smaller PRs to adjust,t hen to make a lot of smaller PR changes and have to revisit this one a dozen times. Let's focus on getting this one finalized and committed, and then worry about the impact the the others.

Ok, we can make this way. Thank you

I added some high-level comments to specific lines of code. I haven't dug into specific testing yet but I will be doing that next.

I also wanted to ask about a potential change to the way 'shopping carts' are use, and if what I propose below is something we can implement since we're doing many changes here anyway.

The issues with the shopping carts are as follows:

• when you click on it, you can't specify if it should be an exclusion or use descendants
• if you click on a 'lit' shopping cart, the concept is immediately removed from the concept set, and if you click it back 'on' (as if it was a mistake to click it), the concept is returned to the concept set as the default 'include, without descendants' settings, which may not be the same state as when it was removed.
• Although the conceptSet data-structure supports having the same concept ID with different exclude/descendant settings, the shopping-cart pattern of adding/removing means you can't do that (the shopping cart enforces only a unique concept per conceptSet). The reason for having the same cocnept ID included twice is that the vocabulary may have a certain ambiguous concept (such as 'other psoriasis skin disorder') map up to a general concept of 'Psoriasis' while other specific source codes map to the many lower-level types of Psoriasis codes. To make a concept for this, you'd pick 'Psoriasis, include descendants' and 'Psoriasis, exclude', which would bring in all the descendants of Psoriasis, but NOT the generic 'unspecified psoriasis' concept ID which those ambiguous source concepts map to. In order to do it with shopping carts, you'd have to exclude Psoriaisis, include Psoriasis-subtype-1...Psoriasis-subtype-N descendants, which is cumbersome when there are dozens of sub-types.

So, what I am suggesting is not a massive (but it is definitely work) from shopping carts to checkboxes where the user is presented the search results with checkboxes (and not shopping carts) and the user selects the concepts they want to add to the 'activeConceptSet', and decides to add them as 'with descendnats' or 'as exclusions' (or both). We'd still use most of the current implementation that keeps track of the ConceptIDs that are in a concept set (this is currently used to determine if the Shopping Cart is 'lit' on a given row of the search results). Instead, we can highlight the row of the table if the concept exists in the concept set, but drop shopping carts in favor of checkboxes. And the user can choose what to do with the selected concepts: add (as exlusion/with desendants) or remove. This prevents the accidental 'unselect-select' issues that I referred to above.

This will also mean we can remove those global handlers on when the shopping cart is clicked. Instead, the concept set editing is encapsulated in the context of the concept list that we're presenting (whether it be from a vocabulary search, or the 'included concepts').

If this sounds simple, let's go for it. If you prefer that we just get the basic functionality implemented and merged, we can do that first, and revisit shopping carts later.

I ran across an error that I think is new since I last checked the origional PR:
require.js:34 GET http://localhost/atlas/js/version.js net::ERR_ABORTED 404 (Not Found)

I don't see js/version.js in master branch, and i don't see a version.js file in the set of files for this PR...what is version.js?

Edit: nevermind: found it: there's a new npm command 'genversion' that produces this file, and I had to generate it via npm run genversion. Mental note on 2.8 release notes: this needs to be called out.

@chrisknoll Generation of version.js was done in #2134

As for you proposal, i think we should implement checkbox in this PR so I think it's better to wait final version to review this. I looked through the code once again and found some issues with.

I have also several questions about implementing checkboxes.
1)As far as i understood the main idea is to replace shopping cart icon with checkboxes(descendants and excluded as we have in concept set expression table in the right columns. What about mapped checkbox?).
2)Vocabulary > Concept page also has shopping cart near the title.
3) We have shopping cart and checkboxes in concept set expression tab. Maybe we can replace this cart with remove icon and if user clicks on it to show alert - Are you sure to remove concept?

Excellent questions :)

1)As far as i understood the main idea is to replace shopping cart icon with checkboxes(descendants and excluded as we have in concept set expression table in the right columns. What about mapped checkbox?).

yes so the permutations of exclude-descendants-mapped is too many to call out individually, so I'd suggest a UI like this:

Clicking 'Add to Cocnept Set' will add all the selected concepts to the concept set with the desired options.

2)Vocabulary > Concept page also has shopping cart near the title.

Right you are! I thought something like this (if you have a selected concept set):

Note: I didn't remove the shopping cart here because I'm lazy, but the idea is that you just have the option to add the single concept to the concept set with the desired options.

3. We have shopping cart and checkboxes in concept set expression tab. Maybe we can replace this cart with remove icon and if user clicks on it to show alert - Are you sure to remove concept?

Right, that's another problem with the current behavior...you can click on it and it just disappears without warning! If we replaced those with checkboxes, then you can even support a 'select all' letting the users choose them all at once, then maybe unselect a few that they don't really want to get rid of, and then a button on the UI will be to 'remove selected'. Once apon a time, we did supoport a 'select all' in the column heading but that led to a 'remoe all from conceptsets' without warning...which is bad.

I'm in complete agreement that this may be more than we want to do for this PR so instead we can do it in a phase 2. But it's good to get our thoughts on the table now so that we don't do anything we regret later.

Thanks. It becomes more clear to me. So actually we have 2 tasks to do with these shopping carts:

1. We have table at search page which is quite similar to the table at the "included concepts" tab in conceptset edit page. There are also same tables in another pages like "related concepts" and "hierarchy" at concept page(maybe more) but i think this is a shared component. There we implement smth as it is on your first picture. I think it is better to move it to the last columns so the user read row => about concept and then decides which checkboxes to select and clicks button "add to concept set"
2. "Concept expression". There we just replace shopping cart with checkbox(not related to checkboxes above in the previous part). User selects which concepts he wants to remove and then clicks button "Remove selected concepts from concepts page".

Just a small question about mechanism of storing active conceptsets. As you could see from the code. I decided to move with current realisation where we have only one ConceptSet in the state and store another active/opened conceptsets in another variable(for now HashedConceptSets). So we have some keys for pages that have conceptsets functionality(repository/cohort-characterization/feature-analysis/etc). So when we move from one page to another. The activeConceptSetSource will change which leads to saving currentConceptSet data to HashedConceptSets. Then we just replace currentConceptSet with new one from HashedConceptSets. When we close some entity - conceptset will be removed from HashedConceptsets object and new CurrentConceptSet will be repository or the first from the list in HashedConceptsets. Is it ok for you?

P.S. Actually i've already started to work on implementing checkboxes) so if you don't mind let's just finish this in one pr

You're correct, there's 2 contexts: checkboxes for 'lists of concepts' (vocab search results, included concepts, mapped concepts), and within the context of a concept set, the checkboxes are to select 'conceptset-items' for removal. Both featuers would be good for this PR, but they are handled differently.

Just a small question about mechanism of storing active conceptsets.

I thought about that too, and saw the references to 'HashedConceptSets', which I didn't quite understand, but figured there was a greater purpose, so I left it alone. But, on the topic, here's what I was thinking:
Referring back to that ugly UI above:

What if we used a similar (note: not exact, the following is a slight copy-paste with different behavior) UI element as we see in cohort definitions for selecting concept sets:

What if both ideas were combined, that instead of 'Add to Concept Set' it would actualy be a dropdown-button defaulting to the last selected concept set, but with the option of the user to choose a different 'active' concept set. If the user chooses another from the dropdown, that selected concept can be come the new 'active' one. Note, this is just for the search results UI where you're not in any 'one' particular left-nav. I think that if you're in the 'included concepts' tab of a concept set expression, there is no choice (and I dont' think it makes sense to give the user the option to choose a set of concepts to insert into a concept set but to a DIFFERENT concept set then the 'included concepts' is showing for. So I am only really speaking about the case where you go to the vocabulary search screen, and you have had multiple conceptsets open.

As far as 'storage', i thought it was going to be as simple as keeping an observable() and an observableArray() (as a computedObservable) in atlas-state:

// atlas-state.js

state.currentConceptSet = ko.observable();

state.feAnalysisConceptSet = ko.observable(); // will be set when opening up a cocnept set for a FE analysis
state.ccAnalysisConceptSet = ko.observable(); // will be set when opening up a cocnept set for a CC analysis
state.irAnalysisConceptSet = ko.observable(); // will be set when opening a IR analysis  concept set
/// ... etc for the different concept sets 

state.activeConceptSets = ko.computed(() => { ... return an array of only non-null concept sets });

Then the dropdown is bound to the set of state.activeConceptSets, and the default is the one that's in state.currentConceptSet.

Just a side-note on the 'concept set picker': that UI depends on the concept set items' isIncluded, isDescendant, isMapped' properties to be observable because the checkboxes in the UI are bound to those observables. But, part of this PR I believe changes that behavior, so just want to make sure that's not a breaking change. Also, I think I saw some code that seems to be 'unwrappiong' observables by hand, but if you just want an 'unwrapped' javascript object from an object with observables, jsut call ko.toJS() on the object, and you get back the raw JS object (with all the object references from the observables intact). Forgive me if I misunderstood what I saw in code, it could be that the unwrap is only happening for purposes of hashing, so in that case, ignore me.

@chrisknoll - from our offline discussion, the changes here look great! I noted a few items that I'm hoping @vlbe can resolve so we can merge this in right away:

• Can we add some visual feedback when pressing the "Add To Concept Set" button so that the user knows their selection worked? Ideally it could be a message that is shown and fades out after a few seconds but I'm flexible on how this is done.
• I'm getting an error like the one below when running npm run build. Here is an example:

[BABEL] Note: The code generator has deoptimised the styling of undefined as it exceeds the max of 500KB.
{ Error: SyntaxError: unknown: Support for the experimental syntax 'classProperties' isn't currently enabled (257:15):

  255 |     // produces a closure to wrap options and source around a function
  256 |     // that accepts the source selected concepts list
> 257 |                 addConcepts = (options, conceptSetStore = ConceptSetStore.repository()) => (conceptsArr, isCurrentConcept = false) => {
      |                             ^
  258 |       const concepts = commonUtils.getSelectedConcepts(conceptsArr);
  259 |       const items = commonUtils.buildConceptSetItems(concepts, options);
  260 |       conceptSetUtils.addItemsToConceptSet({items, conceptSetStore});

Add @babel/plugin-proposal-class-properties (https://git.io/vb4SL) to the 'plugins' section of your Babel config to enable transformation.
If you want to leave it as-is, add @babel/plugin-syntax-class-properties (https://git.io/vb4yQ) to the 'plugins' section to enable parsing.
In module tree:
    main
      pages/Router
        pages/main
          pages/concept-sets/index
            pages/concept-sets/routes

And here is the code reference for the error above: https://github.com/OHDSI/Atlas/blob/issue-1595-fa-concept-set/js/pages/concept-sets/concept-manager.js#L257.

@olga-ganina - If there are any questions on this items or others this week, please direct them to me. Thanks all!

@vlbe - thanks for making these updates. The feedback after pressing the button works well and I was able to use the npm run build command without issue.

I had a question while reviewing this PR. Here is my scenario: I've opened a cohort definition in ATLAS that contains > 1 concept sets. I've navigated to the Concept Sets tab and opened an existing concept set. I then use the "Add Concepts" button which then brings me to the vocabulary search page. I search for a term and when the results are shown, I can select a concept to add to the concept set. I would have expected that the "Add to New Concept Set" section (shown below) would contain a drop down for the concept sets in my active cohort definition? Do I have this correct? To be clear: I'm not requesting changes but just want to make sure if I understand the current behavior and if that is aligned to the design discussions you've had with Chris.

@anthonysena , I think you have a real issue (and this behavior was working once, but with the conflicts and merges, something may have broken, so thank you for calling this out):

The proper behavior is on the vocabulary search results page, it should give you the dropdown for any open concept sets, or if there's none, it should give you the 'new concept set' option. When clicking 'add concepts' from the entity's concept set (such as cohort def, cc, feature analysis, or IR), that concept set (where you clicked add concepts) should become an 'active' concept set and be pre-selected in the choices of open concept sets. So, something is not working right.

If @vlbe doesn't get to it first, I'll figure out what's happening.

@chrisknoll @anthonysena Pushed a commit to fix this. As we discussed with @chrisknoll i added isEditable field to ConceptSetStore to filter out conceptsets from dropdown if entity which contains concept sets is "read only".

Thanks @chrisknoll and @vlbe for this. I pulled down the recent change and now I'm able to toggle between concept sets but now face a challenge when adding the concept set. I'm testing this with security disabled so that could be part of the reason. Here is the display where I've selected 2 items from the search - the "Add To Concept Set" button is disabled despite having selected some items to add:

@anthonysena Hm, that's strange. I've just disabled security and 'Add to concept set' button is active. Could you please provide steps to reproduce it.

@anthonysena fyi, the last commit is fixing another 2 issues with list of active concept sets in dropdown:

1. When you add concepts and create concept set from the search page, then go to some other page and try to search again (no repository concept set option)
2. When you are in cohort definition's/characterization's concept set tab and select to close concept set and go to search - you will have the text in dropdown "Select Concept Set" with active "Add to concept set button". We need to clear activeConceptSet if user closes it.

Would like to clarify this one:

When you are in cohort definition's/characterization's concept set tab and select to close concept set and go to search - you will have the text in dropdown "Select Concept Set" with active "Add to concept set button". We need to clear activeConceptSet if user closes it.

So, I don't think we do this blindly: if we have 3 different concept sets open (ex: Repository, Cohort Def, IR), if we were in the Cohort Def one and clicked 'Add Concept' which set the sharedState.activeConceptSet to the Cohort Definition's conceptSetStore, and then go to the IR view, and close a conept set there, I think the activeConceptSet should remain the 'CohortDefinition' one. Maybe there needs to be a reference check such that:

// in closeConceptSet()

if (sharedState.activeConceptSet() == this.currentConceptSetStore) {
  sharedState.activeConceptSet(null); // clear activeConceptSet when the closed concept set is the active one.
}

Note: this is just pseudo code, I forget what we called the current page's currentConceptSetStore property...

@chrisknoll I am not sure that we need this check because we are setting active concept set when select concept set in the list https://github.com/OHDSI/Atlas/pull/2132/files#diff-68f52280a6354d182ce786a86e8e5610R167

Here's the steps to reproduce:

1. Open cohort definition, select concept set (CD is active)
2. Open CC, select concept set (CC is active)
3. Open IR, select concept set (IR is active)
4. Navigate to search to confirm IR is default
5. Go to Cohort Definition, go to concept set list, observe there is a concept set already selected. Close it
6. Go to search, and confirm that IR is not selected (it prompts you to select a concept set).

After my latest commit, it will only clear the active concept set store if the closed one is the active one.
The logic is simply: if you close the active concept set, then clear the shared state's active concept set.

Note: only catch is that when you click on the Concept Set navigation item, it re-selects the active concept set to the Repository (unlike other navs where it does not do that). So, I'd not make any changes for this, but just be aware of the behavior.