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

Fix: Logging instances of question bank misuse #174

Merged
merged 7 commits into from
Sep 5, 2022
Merged

Conversation

cahirodoherty-learningpool
Copy link
Contributor

Fixes #173

Fix

  • Logging in place for when question banks are attempting to print more than available

Testing

  1. Create a course with an assessment that uses question banks
  2. Set a bank to display three but only have two blocks/components
  3. Two questions should be printed
  4. Console warning should be seen

@oliverfoster
Copy link
Member

Should probably do one here for when a bank id is used in the blocks that is larger than bankSplits length?

const bankSplits = assessmentConfig._banks._split.split(',');
const hasBankSplitsChanged = (bankSplits.length !== this._questionBanks?.length);
if (hasBankSplitsChanged) {

@cahirodoherty-learningpool
Copy link
Contributor Author

Should probably do one here for when a bank id is used in the blocks that is larger than bankSplits length?

const bankSplits = assessmentConfig._banks._split.split(',');
const hasBankSplitsChanged = (bankSplits.length !== this._questionBanks?.length);
if (hasBankSplitsChanged) {

I'm confused. This function is at the article level, but the Bank ID is per-block.

@oliverfoster
Copy link
Member

oliverfoster commented Aug 10, 2022

Yea, you'll have to get the blocks and check their _quizBankId.

A QuestionBank instance is made for each questionBank id (which is the _banks._split index+1) and the QuestionBank instance becomes a set of blocks relating to that id. If someone configures a _banks._split of [2,2,2] and then assigns a _quizBankId of >3 || <1 || not an integer, the code won't warn, it should probably warn that the assigned id isn't valid, maybe? QuestionBank sets are only made for the indices represented by the length of the _banks._split array, so any non-valid id won't have a corresponding QuestionBank set.

I could see cases where it is desirable to set an id that isn't represented in the _banks._split length, but I'd assume that it would be because you'd want to exclude a finishing bank. This can be represented with a 0, such that [2,2,2,0] should work if you wanted to change the _banks._split but exclude the final bank in the course. That'll need checking obviously.

In general each valid _quizBankId should be represented in the _banks._split length and a warning should be generated otherwise.

js/adapt-assessmentArticleModel.js Outdated Show resolved Hide resolved
@oliverfoster
Copy link
Member

oliverfoster commented Aug 16, 2022

Does this now have your approval? @joe-allen-89

@oliverfoster oliverfoster merged commit 88cdcff into master Sep 5, 2022
@oliverfoster oliverfoster deleted the issue/212 branch September 5, 2022 09:16
github-actions bot pushed a commit that referenced this pull request Sep 5, 2022
## [5.1.3](v5.1.2...v5.1.3) (2022-09-05)

### Fix

* Logging instances of question bank misuse (#174) ([88cdcff](88cdcff)), closes [#174](#174)
@github-actions
Copy link

github-actions bot commented Sep 5, 2022

🎉 This PR is included in version 5.1.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Question banks should warn when being used incorrectly
5 participants