I just want it to be known that today I start upgrading the Quiz module to Drupal 5. If anyone already has some patches for this, feel free to post them here.

Comments

wmostrey’s picture

http://mostrey.be/files/mostrey/quiz-5.tar.gz

This is the updated version for Drupal 5.
Currently the only thing that's not 100% is the layout of the multichoice. The options and check/radioboxes are displayed below eachother, not in a table view. Still working on getting that to work.

I would suggest the current maintainer to branch the current version of HEAD as development version 4.7: I've been running it for some weeks in a production environment and it's working just fine. Feel free to add this version as the development version of Drupal 5. It will make bugtracker easier too.

wmostrey’s picture

Ok I found a solution to this. The issue was that check_markup adds a

around the answer. Webchick pointed to look at the Poll module. They use check_plain. Since currently the Quiz module doesn't allow an input forum for the answers, this is the current fix, with which we currently have a fully functional Drupal 5 version of Quiz :)

Replace
$options[$key] = '

'. check_markup($answer['answer'], $node->filter, FALSE) .'

';

With
$options[$key] = ''. check_plain($answer['answer']) .'';

saml’s picture

Nice that it's up running on 5! =)

webchick’s picture

Does that mean you tested it?

I need some folks to test this before I can commit it. :P

saml’s picture

I just installed it, and basic functionality seems to be working well.

I found one bug though, posted in: http://drupal.org/node/117595

I also noticed that HTML is not parsed for the answer labels when viewing a multichoice node, or taking a quiz, while it is on the result page. (I tried e.g. the <sup></sup> tags), but that's maybe more of a feature request.

Also the user message don't render HTML correctly. One example is:

You don't currently have enough questions assigned to this quiz to support that many questions. Either change the number of questions to <em>1</em> or <a href="/lab/drupal5dev-20070209/?q=node/2/questions">add more questions</a> to this quiz.

I will report any new bugs that I find!

Keep up the good work!, thanks,
// Samuel

goose2000’s picture

This is great news - Thanks Quiz guys. I can now think about moving up to the D5 higher plain!

John A

riverfr0zen’s picture

Nice work, wmostrey. Has this been committed to HEAD yet?

Just installed and it looks like most things are working well. Below are any issues I found:

Folllow up to post #2:
In addition to switching to check_plain, you will also need to change line 86 in quiz.module

drupal_set_html_head(theme('stylesheet_import', base_path() . drupal_get_path('module', 'quiz') .'/quiz.css'));

to

drupal_add_css(drupal_get_path('module', 'quiz') .'/quiz.css', 'module', 'all');

in order to get the module css included in 5.x

wmostrey’s picture

These features and bug fixes have been incorporated in the new version:
http://mostrey.be/files/mostrey/quiz-5--1.tar.gz

jinside’s picture

Title: Quiz module for Drupal 5 » Patch for admin deleting of submissions
Component: Code » Documentation
Category: feature » bug
Status: Active » Needs review
StatusFileSize
new1.29 KB

Patch for admin deleting of submissions. Delete process was using 4.7 style of forms. Renamed and created new function for 5.0 form style. Please test!

Thanks!

riverfr0zen’s picture

Title: Patch for admin deleting of submissions » Quiz module for Drupal 5

I've tested jinside's patch (quiz-5.1-admin-delete_0.patch above) and it fixes the bug as tested on wmostrey's post (http://drupal.org/node/114885#comment-203899)

riverfr0zen’s picture

Damn, I had this whole long post written up, and then hit alt-arrow left :(

Anyway, I spoke to webchick a little over IRC, and she stated that she is reluctant (for good reasons) to create a 5.x branch at this time (see http://drupal.org/node/93855). I personally would prefer a 5.x *dev* branch, since it doesn't seem like the people actively working on this are really interested in developing for 4.x anymore, but oh well.

This is fine, but brings in discrepancies in the normal dev workflow for this 5.x version. Namely, as there is no version context for this 'pseudo-branch', it is not possible for developers who are no longer interested in working with 4.x (HEAD) to submit/resolve bugs/patches for this new version, as those bugs/patches would very likely not be relevant to what is in HEAD.

To mitigate this issue, perhaps we could consider this thread a temporary station for this 5.x version. In any case, this seems to be what we are doing anyway. I suggest developers continue to post patches/diffs to this thread, and also adopt/test any fixes others post. Testing is most important, as that is what will lead to a 5.x branch soonest, which is what I think everyone would like.

As it seems wmostrey has taken it upon him/herself to compile changes posted here to and provide builds (have you? heh), I suggest also adopting these builds posted. I suggest these builds are considered to be the 'cumulative' versions, the versions that determine the 5.x Quiz's RTBC-ness, if you will.

Also, there are several patches posted for HEAD that seem to have languished for some time and are still to be tested. I suggest we adopt those patches into 5.x (please indicate where they come from and what they relate to).

Anyway, these are just my thoughts - would appreciate any of your thoughts/input.

hba’s picture

A 4.7 *dev* branch would be really nice as well, just to make the cutoff before 5.x.

saml’s picture

StatusFileSize
new15.57 KB

Tested jinside's patch at http://drupal.org/node/114885#comment-205087
Deleting of quizzes in the admin page now works fine.

The issue http://drupal.org/node/114885#comment-194150 about HTML tags in quiz answers on taking quizzes still doesn't work though. HTML-tags (like <sup></sup>) work when reviewing the issue, but not on taking it. Screenshot attached.

webchick’s picture

@hba: I'll be making a 4.7 branch once this issue is RTBC.

So again, please test and fix errors.

wmostrey’s picture

Okay I finally fixed the issue and brought back check_markup instead of check_plain:
http://mostrey.be/files/mostrey/quiz-5--2.tar.gz

This includes the patch at http://drupal.org/node/114885#comment-205087 and all previous patches.

I fixed it by changing

$options[$key] = ''. check_plain($answer['answer']) .'';

to

$options[$key] = '

'. check_markup($answer['answer'], $node->format, FALSE) .'

';

and adding the following to quiz.css:

div.multichoice_answer_text p{
display:inline;
}

saml’s picture

http://mostrey.be/files/mostrey/quiz-5--2.tar.gz tested and works fine.
Answers now use the same input format as the main body. Precisely what I was looking for. Thanks!

deekayen’s picture

Status: Needs review » Needs work

I think package = Quiz should be added to the .info files.

deekayen’s picture

  • Create content links aren't ucfirst()
  • quiz_settings() isn't updated to http://drupal.org/node/64279#hook-settings recommendations
  • I created a quiz and a multichoice question, then tried to take the quiz and got Field 'score' doesn't have a default value query: INSERT INTO quiz_result (rid, quiz_nid, uid, time_start) VALUES (1, 1, 1, 1172693258). Should default be 0? If so, quiz.install should have score TINYINT NOT NULL DEFAULT 0 for {quiz_result} creation.
jinside’s picture

Ok.. I am little confused can someone set me straight?

Doesnt "$options[$key] = ''. check_markup($answer['answer'], $node->format, FALSE) .'';" enter the possibility of XSS attack for a site that does not want any html content on their site?

Shouldn't it be "$options[$key] = ''. check_markup($answer['answer']) .'';" and then enable a filter with HTML content for questions and quiz module content types to display html content?

I could be off base.

riverfr0zen’s picture

StatusFileSize
new7.73 KB

Patch for the following reports above by deekayen:

http://drupal.org/node/114885#comment-205544
http://drupal.org/node/114885#comment-205556

Besides the items mentioned in those posts, I also did some general renaming of some literals to aid clarity/descriptiveness. You can diff the .module files to see those changes. Let me know what you think, and please do test. Thanks.

riverfr0zen’s picture

As the name implies, the above to be patched over wmostrey's 5.2 build here:
http://drupal.org/node/114885#comment-205346

riverfr0zen’s picture

StatusFileSize
new8.67 KB

I noticed a bunch of mistakes with the default quiz name that I had made to the last patch I posted
(http://drupal.org/node/114885#comment-205872)

Here is a follow up patch that should fix those - please apply the patch above first, then apply this patch. There are also some additional fixes - mainly in the area of using the correct placeholders in the various calls to t().

riverfr0zen’s picture

StatusFileSize
new7.08 KB

5.x port of the 4.x quiz_fatalerrors.patch at http://drupal.org/node/100066

Additionally, I made the following changes.

*Refactored so calls to arg(1) use constant QUIZ_NID_ARG.
*Re-enabled quiz_view override of the view, using 5.x recommendations.
*Added 'Edit' link to theme_quiz_question_table()

wflohr’s picture

StatusFileSize
new18.98 KB

i merged the patches from riverfr0zen

http://drupal.org/node/114885#comment-205872
http://drupal.org/node/114885#comment-205929
http://drupal.org/node/114885#comment-206387

and my own patch (showing quiz results doesn't use table aliases) and attached it to this post

Brendanmcp’s picture

Status: Needs work » Active

Hi everyone...I'm a Drupal newbie and I just thought I would give some feedback on my use of the Quiz module for Drupal 5. It works perfectly in IE for me (the admin), but not in Firefox and not in either browser for anonymous or authenticated users. I have all permissions set but for anonymous users it doesn't jump to the next question and for authenticated users it goes to the next question but I get the following in red:

Validation error, please try again. If this error persists, please contact the site administrator.

Here is the site. http://votingspot.com/are-you-american-idol-expert

Thank you for all your work on this and I'm looking forward to using the final version!

webchick’s picture

Status: Active » Needs work

resetting status

EduardoMercovich’s picture

Hello everybody.

I would like to contribute to this 5.x module. Since I can't code (I don't know how), I can test it. So -to us dummies that don't know about patches, code, programming, etc.- what package/version/tar-gz/whatever should I download and install?

Thanks in advance, and please understand my (limited) knowledge and position. :-)

Best regards...

riverfr0zen’s picture

Hey there Eduardo - I'll try to put up an archive of latest stuff with everything merged (and some additional changes) later tonight.

riverfr0zen’s picture

Eduardo - sorry it's late - here's an archive with the latest patches and some other minor fixes. Let me know if you have any problems. Would really appreciate if you (and anyone else) could test it out.

http://irfanbaig.com/devbin/quiz-5.3-riverfr0zen.tgz

curtisc@drupal.org’s picture

Is this going to be added into the CVS for the module?

webchick’s picture

wmostrey’s picture

I've been testing #29 and all works perfectly. I do have one comment: I was trying to add labels to this version, but it appears that, while checkboxes have a unique id, radio buttons are not provided one. I will try to fix this myself if I find the time next week but I believe this should be fixed before rolling out.

wmostrey’s picture

An update on this: it seems to me the only way to add unique id's would be to call http://api.drupal.org/api/5/function/theme_radio. Perhaps we could include labals, that work for checkboxes but if a user want labels enabled for radios, he should add a snippet to his template.php? I don't know if that's acceptable. Or is there a way to call that function from within the module?

wmostrey’s picture

It's a html input format's <p></p> wrapping around the answer that stops the label from working. So this works for any other format, or in the case the input format wouldn't wrap the text. I think the cleanest option would be to say it's "by design". If everyone agrees: RTBC.

add1sun’s picture

I tested this out briefly and seems like everything is basically working. Good job on moving everything up to 5!

That said, there are a lot of code style issues and I found a few instances of theme('placeholder') being passed into t() functions (or actually called from within the t()?!?) that led to screwy output. I'm also not a fan of all the new constants, particularly QUIZ_NID_ARG. Maybe it is just me, but I actually really prefer arg(1) in the code.

I hope to get more time to review code and test in the next few days. Is the idea that RTBC means "good enough" to not break a site, just so we can get the branch? It definitely needs some cleanup for a normal RTBC. I won't have time to clean and patch until next week but am more than willing to do that.

riverfr0zen’s picture

Still a little new to Drupal way of things - sorry about the QUIZ_NID_ARG - I think you're right on that one, since those values are not constant across function calls. My bad - I'll remove them in my next code release, unless someone beats me to it. That use of non-descriptive numbers still makes me wince though. It makes code more unreadable.

But I'd disagree about most of the others constant. Constants for perms, for e.g. allow ease of changing those perms in the future. Not only in the module in question, but across dependencies too. Especially in a component this young, but even later.

Using constants to replace arbitrary or volatile literals is quite useful. It is a basic tenet of refactoring and provides not just more readable, but also more easily modified code.

add1sun’s picture

Hm, well I still think that things like ('QUIZ_DEFAULT_NAME', 'Quiz') are unnecessary since the default name of 'quiz' is pretty straight-forward and the chances of that being changed over time is pretty much slim to none I would imagine. Also, since it is a stored default variable people can (and will) easily change in the UI, I just don't see any real advantage here.

But those are just my thoughts and webchick can sort out yea or nay on that.

webchick’s picture

I'd say if the thing works without majorly imploding your site, go ahead and mark RTBC and I'll branch. We can sort out auxiliary issues in subsequent patches. In general though, I agree with add1sun that we should use the standard "Drupal way" of dealing with things like configurable variables and URL arguments.

riverfr0zen’s picture

I feel that's quite an assumption you're making that something won't be changed over time (for e.g. I changed those values within the last couple of mods - they were lower case).

Even if the frequency of change is low, why wouldn't you code in a way that will make it easier when a change *is* required? It isn't as though adding a constant will overwhelm resources, nor should it break the application, because of the module name prefix (QUIZ_). I really can't see any downside to it.

Also, using a constant for a default literal has nothing to do with user configuration. It serves the developer, in terms of understanding code and ease of modification.

Of course, I don't want to break any framework conventions, and as I admitted, the node id constant was a silly mistake. But the rest of the stuff doesn't really break any conventions. Just seems like no one else has done it before. Unless I am missing some discourse/documentation (let me know where, if I am).

webchick’s picture

I haven't looked at the code, but I assume that you're using that define so that people can change all instances of the word "quiz" in the UI to "test" or "banana."

The t() function in conjunction with the locale module is how this is generally done in Drupal. You simply create a new "English" language and replace your custom strings in there and you're good to go.

If there's been a big enough call to change this in the UI (I don't remember anyone but like one person suggesting it, but I also haven't been following closely), then you could also do something like add in an option on the quiz settings page for this (much like we allow people to change the title of the Title and Body fields on nodes).

But having it as a define strikes me as non-optimal; in order to change it, I have to go in and edit the code. When I update to the next version of quiz module, I have to remember my change and port it. I'd like people to be able to customize quiz module without having to change the source code.

riverfr0zen’s picture

No, you're misunderstanding. As I said, it has nothing to do with user configuration - it's for developer purposes. Obviously, you wouldn't use a constant to define something for users to change. Generally, I hope one wouldn't even expect a user to open the source.

e.g. calls to:

variable_get('quiz_name', 'quiz') // Second param being the default value

were some things I changed to

variable_get('quiz_name', QUIZ_DEFAULT_NAME)

So, for developers, if you wanted to change the default value, all you need to do is modify the constant value. So much easier than going through the entire source.

add1sun’s picture

Status: Needs work » Reviewed & tested by the community

Yeah, all I meant was that the default in the code is so straight-forward and it is just a default that ends up easily changed by the user on the settings page so - I guess I mean I don't see that there is a lot of fretting to be done on changing a variable_get default that makes a constant necessary. I'm not very experienced with code, so in the end it doesn't really matter to me, I'm just not used to it from other Drupal code I have looked at. I have no idea about whether it makes sense to use on constants on perms but I've never seen that anywhere else in Drupal code either, so it just strikes me as weird.

We can discuss/decide more on this once we get our branch. ;) My test site is not a smoldering ruin so I'd say the code as is works well enough so I'm marking it RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and branched. Awesome work, folks. :) Thanks for all of your hard work!!

I've marked this issue fixed. Please post any additional weirdness clean-up bugs as separate issues.

wmostrey or riverfr0zen: if either of you (or both) are interested in taking on maintainership of this module, please post back here and I'll make it happen.

riverfr0zen’s picture

Yeah, I wouldn't mind maintaining it - although, since I'm quite new to the Drupal world, it would be nice to have someone else co-maintaining too. Regardless, I certainly would like to be on the dev team.

wmostrey’s picture

I'd be happy to co-maintain it with riverfr0zen :)

webchick’s picture

I've added wmostrey to the list, but riverfr0zen, it says you don't have a CVS account yet... please go to http://drupal.org/cvs-account and request one, saying that you wish to be a co-maintainer for Quiz module, and reference this issue.

How this co-maintainership has worked in the past is, when you find something you want to change, make an issue, attach a patch. As long as at least one other person looks at it and gives the thumbs-up, feel free to commit it. :)

riverfr0zen’s picture

thanks, did done that - just waiting till the application is processed.

riverfr0zen’s picture

Hey webchick - looks like my CVS application came through, so whenever you get a chance ... thanks

webchick’s picture

done!

killes@www.drop.org’s picture

Status: Fixed » Closed (fixed)
gingerjoos’s picture

The releases page for Quiz module ( http://drupal.org/node/26481/release ) does not seem to have the old (unsupported) Drupal 5 version. Am I looking at the wrong place? Where can I get the old Drupal 5 version?

mbutcher’s picture

It was removed from the official downloads for security reasons (there is a known vulnerability that none of us has the time to patch). You are welcome to check it out from CVS, patch it up, and use it.

  • Commit c310f4d on 5.x-1.x, 5.x-2.x, 6.x-2.x, 6.x-3.x, 6.x-4.x, 6.x-5.x, 6.x-6.x, 7.x-4.x, master, 7.x-5.x by webchick:
    #114885 by wmostrey and riverfr0zen, plus a bunch of testers: Updating...

  • Commit c310f4d on 5.x-1.x, 5.x-2.x, 6.x-2.x, 6.x-3.x, 6.x-4.x, 6.x-5.x, 6.x-6.x, 7.x-4.x, master, 7.x-5.x, quiz-pages by webchick:
    #114885 by wmostrey and riverfr0zen, plus a bunch of testers: Updating...

  • Commit c310f4d on 5.x-1.x, 5.x-2.x, 6.x-2.x, 6.x-3.x, 6.x-4.x, 6.x-5.x, 6.x-6.x, 7.x-4.x, master, 7.x-5.x, quiz-pages, 2269219 by webchick:
    #114885 by wmostrey and riverfr0zen, plus a bunch of testers: Updating...

  • Commit c310f4d on 5.x-1.x, 5.x-2.x, 6.x-2.x, 6.x-3.x, 6.x-4.x, 6.x-5.x, 6.x-6.x, 7.x-4.x, master, 7.x-5.x, 2269219 by webchick:
    #114885 by wmostrey and riverfr0zen, plus a bunch of testers: Updating...