Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ItemsQuestionModel inheritance duplication #2983

Open
danielghost opened this issue Dec 3, 2020 · 1 comment
Open

ItemsQuestionModel inheritance duplication #2983

danielghost opened this issue Dec 3, 2020 · 1 comment

Comments

@danielghost
Copy link
Contributor

danielghost commented Dec 3, 2020

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

@oliverfoster
Copy link
Member

oliverfoster commented Dec 3, 2020

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 oliverfoster added this to Need fixing in The Quick TODO Board via automation Dec 7, 2020
@oliverfoster oliverfoster added this to New in Infrastructure via automation May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

2 participants