Page MenuHomePhabricator

Submit button on a single quiz grades all quizzes on the page
Closed, ResolvedPublic

Description

The commit I69d95d153d6d1da1e019ae5ad2c3e7a4667971b5 for some reason now causes all quizzes on a page to be graded at once. You can verify this yourself by checking out the previous commit, verifying that only one Quiz is graded when you hit the submit button on a wiki page containing multiple quizzes, and checking out the later commit and verifying that all the quizzes are graded once any one submit button is hit. You should fix this bug.

Getting started

Install mediawiki locally. It is easiest (often) to use vagrant for this. Quiz is not available as a role in vagrant, so follow these directions to manually add lines to LocalSettings.php in vagrant, and include the line

require_once("$IP/extensions/Quiz/Quiz.php");

You can test that the extension is working by adding a sample quiz to your test wiki in wikitext.Y ou can export and then import the Help page from wikiversity for more comprehensive testing: here.

You can find the quiz extension in the vagrant repository by navigating to mediawiki/extensions/Quiz. If you haven't already, you will need to set up git, gerrit, and git-review, and install a gerrit hook to begin submitting commits to the Quiz extension.

Related Objects

Event Timeline

Mvolz created this task.Dec 5 2016, 2:35 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 5 2016, 2:35 PM

@Reedy @Harjotsingh I think this might have been when the CSS was moved.

Mvolz triaged this task as Unbreak Now! priority.Dec 5 2016, 2:37 PM
Restricted Application added subscribers: Jay8g, Luke081515, TerraCodes. · View Herald TranscriptDec 5 2016, 2:37 PM
Mvolz added a comment.Dec 5 2016, 2:39 PM

Yup, narrowed it down to I69d95d153d6d1da1e019ae5ad2c3e7a4667971b5

Reedy added a comment.Dec 5 2016, 2:45 PM

Yup, narrowed it down to I69d95d153d6d1da1e019ae5ad2c3e7a4667971b5

This... Doesn't seem to make sense. How does moving some CSS cause it to grade all quizzes? There's nothing in the CSS that's specific to a single quiz, it's all kinda generic... And on the previous one, the same CSS would end up in the head many times too...

Mvolz added a comment.Dec 5 2016, 2:51 PM

Yup, narrowed it down to I69d95d153d6d1da1e019ae5ad2c3e7a4667971b5

This... Doesn't seem to make sense. How does moving some CSS cause it to grade all quizzes? There's nothing in the CSS that's specific to a single quiz, it's all kinda generic... And on the previous one, the same CSS would end up in the head many times too...

Beats me! That's the assumption I made (that the quizzes would have different CSS and that would affect things) and that's why I tested that change, but as you say, that's incorrect. Possibly something else got messed up/deleted erroneously?

Reedy added a comment.Dec 5 2016, 2:57 PM

The CSS moved in https://gerrit.wikimedia.org/r/#/c/320064/ has nothing specific to a quiz at all. A page can't have varying contlang, and a the colours were effectively hardcoded via an array

I presume, the JS relies on some really weird and wonderful hack...? I don't know how this change would've broken it though -- though, I'm not saying you're wrong that this change did

I just noticed the php below... So it only output it for the first quiz anyway...

# Ouput the style and the script to the header once for all.
		if( $this->mQuizId == 0 ) {

The changes in Quiz.class.php might have caused this.
Maybe the code relies on $wgOut->addScript( $head ) which was removed.

Reedy added a comment.Dec 6 2016, 1:34 PM

The changes in Quiz.class.php might have caused this.
Maybe the code relies on $wgOut->addScript( $head ) which was removed.

No. The code is just using the addModule, and the CSS was put into that module; https://github.com/wikimedia/mediawiki-extensions-Quiz/blob/master/Quiz.class.php#L111-L115 which includes that CSS file already https://github.com/wikimedia/mediawiki-extensions-Quiz/blob/master/extension.json#L21-L24

Mvolz lowered the priority of this task from Unbreak Now! to High.Dec 28 2016, 5:25 PM
Mvolz added a project: Google-Code-In-2016.
Mvolz updated the task description. (Show Details)Dec 28 2016, 5:31 PM
nikitavbv claimed this task.Jan 2 2017, 3:35 PM

I have tested it for a while. It seems that there are some bugs in grading process and they existed far before I69d95d153d6d1da1e019ae5ad2c3e7a4667971b5 change. For example, if you submit the very first quiz on page, that will cause to grade all tests too. And I can track that bug to very old commits (actually, I still didn't find out where did it appear the first time). Another example: if page contains several quite similar quizes, then the grading process is quite strange: sometimes it checks the quiz which I submited (as expected), sometimes it checks some other quizes on the page (but not all), and sometimes it checks all other quizes on the page.
What I am trying to say: looks like there are some bugs in grading process, which even existsed before the change, we are talking about. That change jusy made one of that bugs more noticable, I think.
Anyway, while working on this task, I am going to fix all issues with grading.

Reedy renamed this task from [REGRESSION] Submit button on a single quiz grades all quizzes on the page to Submit button on a single quiz grades all quizzes on the page.Jan 3 2017, 2:06 AM
Reedy removed a project: Regression.

Thanks for digging into it. Untagging as a regression, as it seems to have been this way for a long time...

Mvolz added a comment.Jan 3 2017, 2:24 AM

Hmm, this sounds like it's a lot more involved than I was originally
thinking.

Looks like I found the problem. Quiz::resetQuizID is called and it resets the $sQuizId, which causes those strange grading issues. If we comment code inside that function, preventing static variable $sQuizId from reseting to 0, looks like we can fix all those problems. However, I still don't understand why does that static function gets called. It is not used anywhere in code. However, there is a hook, which is not used in code too. But that function gets called and causes problems with grading. There aren't any comments explaining why that function and hook are needed. So we can either remove it or try to find out why is it called so often. What do you think?

Reedy added a comment.Jan 3 2017, 10:06 PM
		"ParserClearState": [
			"Quiz::resetQuizID"
		]

It's the hook subscriber for the MW Core hook ParserClearState. https://www.mediawiki.org/wiki/Manual:Hooks/ParserClearState

@Reedy thank you for that useful link!

Well, after some investigation, I finally understood what happens here. To start with, I tried to find out why that hook is called by parser so often (why parser clears its state so often). And the reason was quite simple: each time, when we check question state (parseQuestion function in Quiz.class.php), we add quiz_points wfMessage to the output. And if we take a look at that message, we can see that it contains some wiki markup. And looks like parser resets its state each time it parses such messages (what is logical enough).
To finish, I offer such solution: instead of listening for ParserClearState hook, maybe listen for ParserAfterTidy hook (or something similar to that). That will allow us to reset sQuizId to 0 only after all quizes are processed. What do you think about that?

Change 330620 had a related patch set uploaded (by Phantom42):
Fix bug with grading when several quizes on page.

https://gerrit.wikimedia.org/r/330620

Anyway, I have decided not to waste time and upload a patch with solution I offered. If we find better ways of fixing this, I will update the patch.

Change 330620 merged by jenkins-bot:
Fix bug with grading when several quizes on page.

https://gerrit.wikimedia.org/r/330620

Mvolz closed this task as Resolved.Jan 5 2017, 12:40 PM

Thank you for reviewing and merging! If there will be any problems, or @Reedy has some thoughts on how to make this better, I will return and fix it.