Dirty indicators no longer show up

[chrisknoll]

Expected behavior

When navigating to the IR, PLP, or PLE nav in Atlas, if you do not have an existing item open, you should get the list of items that are available for that module (the list of IR analysis, for example).

When you select something to open, it should then change the left nav icon to a color: green for 'open' and red for 'open and unsaved'. If you select the left nav with an entity open, it should navigate directly to the edit screen for what the user has selected to edit. If the entity is 'dirty', the icon in the left nav should appear 'red'.

Actual behavior

When selecting an IR, PLP, or PLE analysis, the indicator in the left nav no no longer indicates that the item is 'dirty' (a green icon means not dirty, a red icon means unsaved changes). Additionally, after opening an item, clicking back on the nav is supposed to send you back to the item that is open, but as of the 'pages refactoring' the left nav is static, and clicking on the left nav always send you back to the list view of the module.

Because of the lost functionality, if you open an IR analysis, and make unsaved changes, then click on the left nav, you are directed back to the IR Analysis list (browse) you can click on some other IR Analysis, and there's no inidication that unsaved changes were lost, that you had an IR Analysis open, etc.

Steps to reproduce behavior

Go to IR analysis List. Click New Analysis. Note: Icon in left nav is not green Make changes, do not save click Left nav. Note: it takes you to the analysis list (this is wrong, it should have not navigated anywhere because you have an analysis open Click some other IR analysis item. Your unsaved changes are lost, and no notification.

Implementation notes;

From the 2.4.3 branch, you can see the left nav is created statically and looks like this: https://github.com/OHDSI/Atlas/blob/v2.4.3/index.html#L47

            <a class="list-group-item" data-bind="css: irStatusCss, attr: {href: irAnalysisURL}"><i class="fa fa-fw fa-bolt" aria-hidden="true"></i>&nbsp;Incidence Rates</a>
            <a class="list-group-item" href="#/profiles"><i class="fa fa-fw fa-user" aria-hidden="true"></i>&nbsp;Profiles</a>
            <a class="list-group-item" data-bind="css: ccaCss, attr: {href: ccaURL}"><i class="fa fa-fw fa-balance-scale" aria-hidden="true"></i>&nbsp;Estimation</a>
            <a class="list-group-item" data-bind="css: plpCss, attr: {href: plpURL}"><i class="fa fa-fw fa-heartbeat" aria-hidden="true"></i>&nbsp;Prediction</a>

The css and attr bindings are used to control what the user is navigated to when the left nav is clicked. You should also notice: not all links work this way.

Background on the difference in behavior:

The Concept Set and Cohort Definition modules work differently than the other modules: Because of the way the vocabulary manipulates concept sets in the 'open' entity, if you have a cocnept set open or cohort definition open, you have a 'breadcrumb' at the top of the main UI that will let you return to the open cohort def/concept set. Note, this meant that you could only have only either a cohort defintiion OR a concept set open to manage, but not both. Complicated logic was implemented in the app to make this functionality work. You will see things like the 'source' of the concept set being 'definition' or 'repository' and other things like this.

When we added other modules (IR was the first one), we quickly realized that the breadcrumb behavior was not maintainable and very complicated to implement. In addition, it did not make sense to have to close your IR analysis just because you opened up a concept set. Therefore, we coded the app to maintain a state of the open entitities, and had the left nav show colors based on the 'dirtyness' of the entity and the URL to navigate to the module would change to the 'active' entity so if you click on it, you go back to the item you have open.

When the code was refactored into the pages structure, and the left nav is now dynamic based on what page modules have been defined, we lost the functionality of maintaining dirty state of the entity and also the navigation to the open entity in the left nav.

[chrisknoll]

I found this inside the index.js for IR:

      baseUrl: 'iranalysis', // todo: css: irStatusCss, attr: {href: irAnalysisURL}

So it appears that this is an incomplete implementation, so we'll just track that part under this issue.

[chrisknoll]

All, can you please review this commit that was based off master (and is in a branch of it's own): https://github.com/OHDSI/Atlas/commit/b3d78a2b825374d2d464959bc27b04b817946e96

I've updated the way pages are built to support a 'navURL' for the left nav, and also to calculate a 'cssStatus' for showing the indicator of the status of the nav:

Open/not dirty:

Dirty status:

Notice the red/gree inidicator on the left

Part of this change was to remove the state from the root model, and instead allocate a portion of 'atlas-state' module to store those stateful elements that live on outside of the actual component. This could have continued to live int he main app model, but I feel that the app model may be getting a little too cumbersome and perhaps it would be better to leverage the atlas-state component to share state across modules that may not have access to the root model. This also insulates changes in the root model from the state that we want to maintain across components.

These changes to the branch are soemthign I'd like to resolve becuase with the new CC and pathways work, these root issues are leading to issues in the new development (such as, the page definitions and how we track the route state across requests).

Please let me know if you have any questions, or if you'd like to meet in real-time to try to settle on a single approach for this so we can un-block the current development.

[anthonysena]

Thanks for this fix @chrisknoll - I reviewed the commit and the new approach works well for me. Thanks!

[pavgra]

@chrisknoll, thank you for locating the issue and very comprehensive description!

@johnSamilin, additionally to the stated above, would be great to encapsulate the logic for storing opened entities and any other shared state into the appropriate modules (sections) so that the logic doesn't pollute global state and application skeleton. E.g. following code:

appModel.currentCohortDefinitionMode(view);
            appModel.loadCohortDefinition(cohortDefinitionId, null, 'cohort-definition-manager', 'details', sourceKey);             appModel.loadCohortDefinition(cohortDefinitionId, null, 'cohort-definition-manager', 'details', sourceKey);

and following tons of params at app-wide level:

[chrisknoll]

Yeah, be warned: the way cohortdefinition and concept set is coded is very tricky.

Unfortunately, I don't think we can really 'fix' the cohort definition/concept set sections easliy for this relaese. What I woudl focus on is the Incident Rate, PLP/PLE, Cohort Characterization and Pathways work to follow the navigation hooks and we'll have to revisit the exceptions (cohort/conceptsets) later.

[johnSamilin]

I'd say, refactoring of the global model is the subject for a separate step, because, as everyone agrees, it's very complicated and nearly half of it should be moved to atlas-state (or even to a state management library). @chrisknoll there is one thing that I wanted point: https://github.com/OHDSI/Atlas/commit/b3d78a2b825374d2d464959bc27b04b817946e96#diff-eacf331f0ffc35d4b482f1d15a887d3bR131 this change would break the logic because then components would receive an object containing the field componentParams instead of an object containing actual parameters, so there should be additional changes. We did it in another commit https://github.com/OHDSI/Atlas/pull/901/commits/7f3b273445eae7a681611fdbe97add6f0c65f98b and https://github.com/OHDSI/Atlas/pull/901/commits/a4b4603ddf4b96246a2f3206a453db8541a20186

[chrisknoll]

That's correct @johnSamilin , it is a breaking change there, and we'd have to account for that. But that was the intent of the change: that we couldn't merge the different objects together into a single param object using this syntax:

<div id="currentComponent" class="flexed" data-bind='component: {name: $data.currentView, params: { ...$data.componentParams(), model: $data } }'></div>

The spread operator here causes the 'param' object to be rebuilt when any of the observables change (such as during the course of handling a route change). This causes the component to be re-built, which is not the behavior we want.

But, beyond that, you should consider leaving the parameters coming in from the route as a separate parameter object that the 'Page Component' can subscribe to for changes in the route, and handle them accordingly. So in your commit you have this:

    this.cohortComparisonId = params.routerParams().currentCohortComparisonId;
    this.cohortComparison = params.routerParams().currentCohortComparison;
    this.cohortComparisonDirtyFlag = params.routerParams().dirtyFlag;

But instead, you should do this:

    this.routerParams = params.routerParams;
    this.routerParams.subscribe(newRouterVals => {
        // do work here with the new router param object that is set in the observable
        // such as load different entity, switch tab, etc
    }

If we introduced a parent class of 'Page' for these types of components, we could standardize this behavior across all pages.

Also, I'm not sure why the dirtyFlag is being managed by the routerParams(), I don't think that belongs there, but I do understand that there's a need to have individual components manage their own dirty flag, but that's why I put that in the 'shared state' component in my Incidence Rate example.

I was going to suggest that each route for each page component maintain it's own observable for storing the routerParams() because I am wondering if there is a risk of one component being notified about router param changes that is directed to a different routed component.

When we have this in code:

    const characterizationViewEdit = new AuthorizedRoute((id, section, subId) => {
        appModel.activePage(this.title);
        router.setCurrentView('characterization-view-edit', {
            characterizationId: ko.observable(id),
            section: ko.observable(section),
            subId: ko.observable(subId),
        });
    });

Here, setCurrentView would take the second parameter (the object) to write into the routeParams() observable. Then, the component would wake up with that route change and handle it accordingly.

[chrisknoll]

I should also add: the object passed into the second param of setCurrentView() does not need to contain observables themselves. Instead just a static object of values needs to be passed in, and then the subscriptions in each component will recieve those values and act accordingly. I'm not sure what value passing in 'fresh' observables (via ko.observable()) to these values would serve.