adapt_framework icon indicating copy to clipboard operation
adapt_framework copied to clipboard

ItemsQuestionModel inheritance duplication

Open danielghost opened this issue 5 years ago • 1 comments

Due to the BlendedItemsComponentQuestionModel class, there is duplication of the init and reset methods in the inheritance chain. This is more of an issue during initialization than when resetting, as the models will attempt to restoreUserAnswers twice. In addition to being inefficient, this could lead to data being restored incorrectly depending on the logic within that restoration, i.e. anything which increments a counter etc. on a class property would be calculated incorrectly.

Currently the init execution order works as follows, which then leads to the https://github.com/adaptlearning/adapt_framework/blob/master/src/core/js/models/componentModel.js#L57 being listened to twice : ItemsQuestionModel -> BlendedItemsComponentQuestionModel -> ItemsComponentModel -> ComponentModel -> QuestionModel -> ComponentModel

I would assume this issue has been around since v3.3.0. The issue is caused by the inheritance chain being duplicated by the following: https://github.com/adaptlearning/adapt_framework/blob/master/src/core/js/models/itemsQuestionModel.js#L9-L17

With the current class design, I am not sure how can move the QuestionModel call to execute between ItemsComponentModel -> ComponentModel, but it would be possible to prevent ComponentModel from adding the adapt:initialiaze listener twice as a workaround.

We could either add this.stopListening(Adapt, 'adapt:initialize', this.onAdaptInitialize); before the listener is added in ComponentModel, or add a new _isInitialized property to AdaptModel, which returns early, or prevents super classes being called if already initialized. A simple implementation would be: https://gist.github.com/danielghost/b858f803926a626e59b44d7f3b07a56b#file-adaptmodel-js-L30 https://gist.github.com/danielghost/b858f803926a626e59b44d7f3b07a56b#file-adaptmodel-js-L157-L159 https://gist.github.com/danielghost/b858f803926a626e59b44d7f3b07a56b#file-componentmodel-js-L52-L63

danielghost avatar Dec 03 '20 13:12 danielghost

I think I prefer the this.stopListening(Adapt, 'adapt:initialize', this.onAdaptInitialize); idea over the _isInitialized one.

  • init could then be called on the ComponentModel and AdaptModel multiple times whilst keeping a consistent state
  • otherwise as init it being executed, the change:_isInitialized event will be triggered when the AdaptModel.prototype.init is first reached through the first fork of the inheritance chain but the ItemsQuestionModel will not yet have completed initializing the second fork of inheritance, the QuestionModel side and so the state of _isInitialized will not be indicative of the real world state

oliverfoster avatar Dec 03 '20 14:12 oliverfoster