Adapt_framework: adapt-contrib-trickle: "_isAvailable": false on an article causes error

[RobertPeek]

If an article is set to "_isAvailable": false, trickle breaks and you cannot trickle through a page.

Environment

Framework v4.3.0
Latest Chrome
Windows 10

Steps to reproduce

Set up a page with 3 articles, each with trickle configured on the article with "_scrollTo": "@article +1"

Set the second article to "_isAvailable": false

Run course and view the page.

Expected behaviour

Trickle should move through the 2 'available' articles.

Actual behaviour

Clicking Trickle on the 1st article does nothing and the console error is shown

[jamesrea83]

The quick fix for this one is to make sure the child blocks are also set to _isAvailable: false.

Would it be worth having a feature that, upon finding an _isAvailable: false flag, automatically sets all applicable child elements as well? Is there a scenario where you would want an available block contained in an unavailable article?

[moloko]

Is there a scenario where you would want an available block contained in an unavailable article?

@jamesrea83 we have found previously that cascading changes from parent to children can be problematic in unexpected ways. I think @oliverfoster has an alternative technique for addressing this particular issue from within trickle.

[oliverfoster]

Add something like this to adaptModel.js:

  /**
   * Derives isAvailable from parent stack
   */
  getIsAvailable: function() {
    var isAvailable = this.get('_isAvailable');
    if (!isAvailable) {
      return false;
    }
    var parent = this;
    while (parent = parent.getParent()) {
      isAvailable = parent.get('_isAvailable');
      if (!isAvailable) {
        return false;
      }
    }
    return true;
  }

Then use in trickle.

[akshay-bhavsar]

Please ignore above comment and refer PR: #2665 for this issue

[moloko]

this will need fixing in Adapt v5.x/Trickle v4.x as well I think?

[moloko]

Add something like this to adaptModel.js:

  /**
   * Derives isAvailable from parent stack
   */
  getIsAvailable: function() {
    var isAvailable = this.get('_isAvailable');
    if (!isAvailable) {
      return false;
    }
    var parent = this;
    while (parent = parent.getParent()) {
      isAvailable = parent.get('_isAvailable');
      if (!isAvailable) {
        return false;
      }
    }
    return true;
  }

Then use in trickle.

I think this can be done in one line (at least when using ES6):

this.getAncestorModels(true).every(model => model.get('_isAvailable'));

Which makes me wonder if it's worth adding to the core code?

[oliverfoster]

isAvailableInPage is already in isn't it?
You don't want it to descend beyond the first content object because a menu might be _isAvailable:false intentionally

[moloko]

isAvailableInPage is already in isn't it?

I think it's only here? https://github.com/adaptlearning/adapt_framework/tree/v4-patches

[moloko]

I don't mind particularly, it just seemed to me that if it can be done with one line of code then there's no need to do two separate FW releases as well as two trickle releases

[oliverfoster]

this.getAncestorModels(true).every(model => !['page', 'menu'].includes(model.get('_type')) && model.get('_isAvailable'));

or in v5:

this.getAncestorModels(true).every(model => !model.isTypeGroup('contentobject') && model.get('_isAvailable'));

There's no point checking the availability beyond the contentObject as availability is a contentObject relative property.

[moloko]

this.getAncestorModels(true).every(model => !['page', 'menu'].includes(model.get('_type')) && model.get('_isAvailable'));

or in v5:

this.getAncestorModels(true).every(model => !model.isTypeGroup('contentobject') && model.get('_isAvailable'));

I think these will both return false for anything that's a child of a content object?

There's no point checking the availability beyond the contentObject as availability is a contentObject relative property.

That's true - but it's generally only going to mean checking one more thing (the course) so doesn't seem like too much of an overhead

[oliverfoster]

That's true - but it's generally only going to mean checking one more thing (the course) so doesn't seem like too much of an overhead

It's not because it's expensive that you should not check them, it's because any of the ancestor contentObjects could be _isAvailable: false to hide them from a menu or for use only as a container of notify components or a multitude of other reasons.

I think these will both return false for anything that's a child of a content object?

That's true, they should be a filter / similar.

this.getAncestorModels(true).filter(model => !['page', 'menu'].includes(model.get('_type')).every(model => model.get('_isAvailable'));

or in v5:

this.getAncestorModels(true).filter(model => !model.isTypeGroup('contentobject')).every(model => model.get('_isAvailable'));

[moloko]

It's not because it's expensive that you should not check them, it's because any of the ancestor contentObjects could be _isAvailable: false to hide them from a menu or for use only as a container of notify components or a multitude of other reasons.

Ahhh OK I think we're coming at this from two different directions, hence slight confusion. My use-case is to ensure that adapt-search doesn't show results that are included in any 'unavailable' contentObject (or article or block) so it's vital that it does check to see if the parent contentObject _isAvailable or not.

So if you have a different requirement for trickle I think it's an even stronger argument for not including something like isAvailableInPage in Adapt itself and instead having trickle/search do their own check. I'll close the v5 PR.

[oliverfoster]

Or the alternative, that both are useful in the core?

[moloko]

Or the alternative, that both are useful in the core?

From my perspective, it's such a small bit of code that unless it's going to get used by lots of different plugins, it's probably not worth including in core