nicholasThompson and I have been comparing notes on the work we did last week. Here is a patch I built from his module for review. Note that this was built from the current cvs HEAD (after http://drupal.org/node/77792 was applied by webchick).

This does not include any of my patches, and needs to be reviewed to see where there is overlap so we can move forward. For reference, here are mine that are currently waiting for review:

http://drupal.org/node/84419 (completion of workflow for taking the quiz)
http://drupal.org/node/84408 (turning off the default value for quizes and handling the error)
http://drupal.org/node/82753 (Pass or fail feeback at the quiz level)

CommentFileSizeAuthor
#7 quiz_combined4.patch25.5 KBseanbfuller
#6 quiz_combined3.patch25.49 KBseanbfuller
#5 quiz_combined2.patch26.02 KBseanbfuller
#4 quiz_combined.patch25.55 KBseanbfuller
#1 quiz_fromnikos.patch20.14 KBseanbfuller
quiz_working_njt.patch6.62 KBseanbfuller

Comments

seanbfuller’s picture

seanbfuller commented
StatusFileSize
new quiz_fromnikos.patch20.14 KB

Here is a patch based on the version of the quiz module that Nikos sent me. Up for review.

After a quick look through it, seems like there’s a decent amount of overlap between this version and my three patches listed above. I'm going to go through and highlight the conflicts so they can be resolved. Where possible, it probably makes more sense just to use his version, but we probably want to double check the code.

seanbfuller’s picture

seanbfuller commented

I spent some time looking through the patch and I’ve noticed a few issues. Nikos and Nicholas did a great job of moving this forward and there are some great elements in there, but at this point I don’t think it is commit ready. Here’s a quick list of things that I noticed:

1. Hard-coded options

1a. The main thing is the summary feedback message, which looks like this:

That was a good effort, but you will need to take the test again. Because of the importance of Child Protection training, we need to make sure you score 100% in all of the tests.

You may want to print this results page (using the File then Print function on your top toolbar), so you can refer to it when you take the test again. This will help you move quickly through the questions you already got right, and give you extra time to concentrate on the ones you got wrong.

1b. The “take quiz” menu option was changed to “take test.” I think it’s a great idea to be able to change nomenclature across the site, but this should be a setting.

1c. The new “Pass rate for quiz %” is required and the default is set to 100%. Not all quiz creators will want to use a pass / fail control so it should not be required. Also, the default should be a setting.

1d. There is a hard-coded call to theme_add_style('themes/bluemarine/print_style.css', 'print'); This is something that theme admins should be taking care of. It should not be in the module.

1e. Not sure about this one, but I found this comment:

//BE CAREFUL OF THE $_POST['edit'] ARRAY AS THIS VALUE IS LIABLE TO CHANGE WHEN VOCABS ARE ADDED AND DELETED...IF YOU HAVE MORE THAN ONE VOCAB, MAY GOD HELP YOU!!

Nikos, I couldn’t tell if this was something you were playing with or if you were pointing out a bug you found. Looks like it is supposed to help quiz creators filter the questions by taxonomy, but I gave it a try and it didn’t work for me. Not sure if it works in current CVS version either.

2. Interface Issues

2a. Question titles are displayed while taking the quiz. From what I understand of the quiz module at this point, questions are supposed to be added to the question pool, then assigned to a particular quiz. This could cause confusion when a question creator’s quiz gets added to someone else’s quiz. It’s better than the word “title” that was showing up, but I think we want the quiz title instead of the question title.

2b. The progress bar was a bit confusing. The idea is cool, but because of the way it is animated, at first I thought that the quiz module was telling me to wait for it to finish loading before I start. One of our designers made a good point that if there were some sort of milestone markers, it might make more sense, but still the animation is a bit confusing.

3. Drupal Issues

3a. None of the display functions call the theme. (quiz_question, quiz_result and quiz_view)

3b. The code could use a few more comments to help other developers work through it.

4. Things I really liked

4a. I like the layout of the quiz summary page (quiz_result).

4b. The 1-100 validation on the "pass" field is something I missed in my patches. I totally slapped my forehead when I got down to there.

4c. The “my results” menu item is a cool idea.

4d. The "illegal option" bug was fixed.

Given these issues, the somewhat chaotic state of my own patches, and the conflicts and overlap between the two, I wonder if the best way to move forward is for us to create new patches for each issue. This would allow us to use the best code from both sets. It would also keep us from accidentally putting anything in the module that we don’t want. This would push the module back a bit, but since we both have already deployed our module to our respective clients, I don’t see that as an issue.

Thoughts?

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

Wow! What a reply! Ok - from the top...

1a. Probably hardcoded for the sake of a deadline. Obviously - this will be part of the quizzes settings. My opinion: TO DO - HIGH PRIORITY

1b. I agree this should be at least a module setting - if not a per-quiz setting? My opinion: TO DO - LOW PRIORITY

1c. Agreed, it should be an option... Although whats the point in a quiz or test if there is no pass mark? My opinion: TO DO - HIGH PRIORITY (quick fix)

1d. I'm not sure about this... Its a toughy. On one hand I agree that this should be handled by the theme. On the other - this is a module specific print tweak. My Opinion TWEAK TO GET THIS CSS TO BE NON-THEME SPECIFIC, BUT INCLUDED AT MODULE LEVEL

1e. That comment is to warn developers of a "here be dragons" area of drupal. This is the cause/fix of the Illegal option bug. In drupal, if there is ANYTHING in the $_POST['edit'] variable and a form is rendered - the Forms API tries to be clever and fill the form in based on $_POST['edit']. The thing is - with Quizzes, the next questions form is pretty similar to the last except it might have fewer answers. If this is the case, the Forms API might try to fill in position 5 as ticked when there are only 4 position to try. This throws the invalid option error as its trying to fill out the current question based on the selection of the previous one. That was a fix inspired by the Eurostar on the way back from DrupalCon! :-) My Opinion LEAVE COMMENT FOR THE SANITY OF OTHER DEVELOPERS - MAYBE REWORD?

2a. Not sure I understand the issue here...

2b. The animation looks odd doesn't it. The bar makes a LOT more sense if you tweak the theme to use a stationary image. I agree - milestones would help too. No idea how to achieve that though?

3a. And this is a problem, why? How do you propose to solve this (I cant see a problem).

3b. Obviously - this would be a good idea. We were on a tight deadline to get it live so comments are a "phase 2" thing :-)

4a. Looks good doesn't it - the Tables themeing makes things like that a breeze!

4b. Nice touch Nik

4c. That was something we wanted from the begining - much more accessible for the user. The data is recorded anyway, why not give them access to it? :-)

4d. Covered above

For now - I propose the following...
As I think Nikos's one does most of the stuff needed in one patch, this is what I think should happen (imho):

  1. Take a CVS download
  2. Take Nikos's patch (and apply)
  3. Tweak all the little bits that are hard coded to our needs
  4. Submit that as CVS
  5. You patch the hell out of it for your needs but bearing in mind we need to keep it flexible
  6. We submit your version
  7. We ask Webchick for a gold star

Thoughts?

seanbfuller’s picture

seanbfuller commented
StatusFileSize
new quiz_combined.patch25.55 KB

Here is a first pass at combining the work to-date. I have a list of additional issues that will need to be resolved after this patch is applied. For the most part, anything that didn't work before probably still doesn't work.

This removes most of the hard-coded elements and replaces them with settings and node fields.

Note that there are two new database fields. (summary_pass and summary_default).

It's not perfect, but it should be a good start at getting us to a unified version that we can patch against.

Some major points:

  • Removed just about anything that was hard-coded to Niko's environment.
  • Adjusted the correct and incorrect feedback to be themed instead of using images that are not located in the files directory.
  • Added two new database fields in the .install file to hold the summary info.
  • Pass / Fail options are optional.
  • Added validation to the pass_rate to make sure that is is a number
seanbfuller’s picture

seanbfuller commented
Status: Active » Needs review
StatusFileSize
new quiz_combined2.patch26.02 KB

New version that fixes a small bug in the quiz form and removed to extra db lines that I missed.

seanbfuller’s picture

seanbfuller commented
StatusFileSize
new quiz_combined3.patch25.49 KB

Third version that fixes an obvious mistake I made. (Deleted the wrong line.) Thanks to Nicholas for catching these dumb mistakes. Sorry about that.

seanbfuller’s picture

seanbfuller commented
StatusFileSize
new quiz_combined4.patch25.5 KB

Fourth and most linkely final version. Thanks to Nicholas for staying up with me to tease through some of these changes. Here's the quick rundown of additions from the last patch:

  • Removed an extra sql query to get the quiz title that was left over from Nikos's code. This call was made redundant after I added the call to node_load in _quiz_get_results_table and I forgot to remove it.
  • Removed some hard-coded images that Nicholas helped me find and replaced them with calls to a theme function that is currently spitting back special unicode characters.
  • Added check_markup calls to summary_pass and summary_default.
  • One or two slight adjustments to comments.
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work

Woah. Massive patch. :)

1. I see columns added to the table, but no update hook. Do we need to allow for people upgrading from older versions? Or are we just going to say, "it's beta. get over it." ;)

2. Somehow.. and I'll need to try and remember how I did this... I got the following error:

user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '-1' at line 1 query: SELECT question_nid FROM quiz_questions WHERE quiz_nid = 7 AND question_status = 0 ORDER BY RAND() LIMIT 0, -1 in /Applications/MAMP/htdocs/drupal-4-7/includes/database.mysql.inc on line 121.

I believe I had created a quiz, taken it twice, then added a new question to it, then taken it again. My quiz progress indicator also said -100%

3. I'm not real crazy about the upload progress bar.. the swirling bar makes my eyes bug out. ;( Is there a way to stick a solid background colour in with CSS or some such thing?

4. "user results" is not a very descriptive permission name.. "view user results" (or "view quiz results"), maybe?

5. /node/$nid/quiz/start got changed to "take test" from "take quiz" .. since this is called "quiz" everywhere else we should probably call it quiz here as well.

6. I like the inclusion of the quiz summary results and pass mark.

7. Could we get a more descriptive comment of multichoice_calculate_results apart from +//New singing and dancing one? ;) It looks like that function's doing a lot.

8. LOL @ the comment all in caps. :) At some point we should figure out something we can do there, but for now it shouldn't hold up the patch.

9. There are some coding style issues, but I can go in and clean those up some other time.

Marking "code needs work" because of #2. If you could look at the rest of them too, that would be cool, but #2 is kind of a biggie. After that, feel free to commit.

seanbfuller’s picture

seanbfuller commented

Overall, Nicholas and I were just looking to get our work combined into one patch so we could move forward. I have over 20 bugs that I'll post and start working on as soon as I have a solid cvs version to patch against. I didn't want to fill the queue until we were past this point. Most of your points are on my list, but others range from calls to the theme that should be there to items just not working at all.This is by no means a deliverable module, but it is a step in the right direction.

I'll def take a look at #2, but at the same time it would be nice to get over this hurdle so we can start submitting smaller and more easily digestible patches. Either way, I'll start posting my issues tomorrow.

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

Thats an interesting query error! How did you produce that? How many questions were in your quiz?

In terms of update - I wouldn't bother. The module is at such a volatile stage people must be aware (especially with the nice disclaimer on the project page) that its gonna go through a lot of changes.

I think once that query is done the patch should be submitted to CVS and then Sean can start on his bug list :-)

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

3. I'm not real crazy about the upload progress bar.. the swirling bar makes my eyes bug out. ;( Is there a way to stick a solid background colour in with CSS or some such thing?

In short - yes. Thats what we did with our assessment site. A simple CSS fix. Either that or Quiz needs its own CSS file (like Devel does). Then we can style a better progress bar that doesn't eat your eyes.

thetoast’s picture

thetoast commented

First of all I just want to say sorry for all the hardcoded stuff I put into the module, I would have paid more attention to this if we didn't have a deadline to meet. To be honest this was my first attempt at hacking a module, I've only been drupaling for 3 months and working on this module has taught me a lot.....also sorry to Sean for not making myself available to help with taking the module forward....my work load is quite large at the moment and I think that reflects in some of the sloppy code I write ;-) But anyways, I was just reading this page and just wanted to make a suggestion.

5. /node/$nid/quiz/start got changed to "take test" from "take quiz" .. since this is called "quiz" everywhere else we should probably call it quiz here as well.

I understand that there needs to be a consistency with the word quiz, but when it comes to using this module as a serious assessment tool then I think the term quiz doesn’t quite fit the bill. The way I interpret the word quiz is something you do for fun, and although education is meant to be fun (so they say) it is also something serious, such as in the context of child protection that we used the module for. This is why I was told to change quiz to test. Maybe the module should be renamed to assessment or something to that effect………of maybe you could just tell me to shut up and get on with some work ;-)

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

I actually would vote +1 for the change in module title. When I first saw Quiz I linked it with pub quizzes, not with something you could use as an LMS.

seanbfuller’s picture

seanbfuller commented

thetoast (Nikos): No problem. It's all good. :)

I'll try to track down that sql query problem and create a new patch.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

I'm trying to remember what I did. :(

At first, my quiz only had 1 question.

Then I gave it two.

OH!

I wonder if I added another question and didn't increment the # of questions on the quiz in the quiz settings.

Let me test.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs work » Reviewed & tested by the community

Yep. that was it. So don't let that hold up this patch -- kind of an edge case. Eventually we'll want to increment this somehow so that doesn't happen, though.

And grumble grumble at the title change. ;) But fine. Once we get something here we can use, I guess we probably should look at naming this module more descriptively then if that's come up that many times.

nicholasthompson’s picture

nicholasthompson
Primary language English
commented

Or, if we dont rename the module as a whole - we could have a config area to name the assessment type? That way the tab can say:

  • Take Assessmenet
  • Take Quiz
  • Take Test
  • Take Exam
  • Take It Easy
  • LETS GO!!!

Either drop down or free entry?

I'll roll the patch in (I'll try not to break anything!!! :-) )

nicholasthompson’s picture

nicholasthompson
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

I have patched and commited the update... All we need to do now is wait for the site to update itself (which i understand happens 3 times a day.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)