Language Tests

What are the differences to the quiz module? Please add that to the project page.

The branch name should follow Drupal's naming conventions (so it should be 7.x-1.x). Please see http://drupal.org/node/1015226

manual review of branch 7.x-1.x-dev:

.install file

$permissions = array('access toolbar');
user_role_grant_permissions($rid, $permissions);

• hook_install: The above code will return error on installation if the Toolbar module is not enabled.
• I'm not 100% about this but I'd say that it is best practice not to set any default permissions for roles, let the site admins do that in the UI. You could instead write a help text or set a Drupal message upon installation to remind admins to configure the module's permissions.

Also, get a review bonus and we'll get back to you sooner.

Hello, first off used the online auto scanner. Found some issues. These are included in the attached file. Feel free to run the scanner yourself to resolve issues.

For Drupal coding conventions please refer to:
http://drupal.org/coding-standards#comment
http://drupal.org/node/1354

100s of style errors in the attached file.

Looks like you have some of the theme stuff in there, and looks like an interesting mod.

Got time (not at lunch!) to do a more complete install-run-test review below at http://drupal.org/node/1399684#comment-5672690

do NOT wrap text in hook_menu functions with t()!

minor issues, switching back.

@patrickd There were hundreds of issues with the
Review of the 7.x-1.x branch:
I didn't know that all of these were totally minor such as these found:

+1182: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar.

<strong>sites/all/modules/pareview_temp/./test_candidate/langtests.install:
+385: [normal] Use st() instead of t() in hook_install(), hook_uninstall() and hook_update_N()
</strong>
10s of (minor, but many)
+1204: [minor] in most cases, replace the string function with the drupal_ equivalent string functions

116 | ERROR | Use XHTML style tags instead of
439 | ERROR | Use XHTML style tags instead of
546 | ERROR | Use XHTML style tags instead of
...

100s of
839 | ERROR | Array indentation error, expected 8 spaces but found 0

Again thanks for the new information.

I am surprised with the security issue, tag issues, and the 100s of formatting issues this should not be set to needs work - your call, glad to be learning the process. Thanks again.

yes, but if you have a look at the critical line:
drupal_set_message(t('Please, !login or !reg before attempting to join the group.', array('!login' => l('log in', 'user'), '!reg' => l(t('register'), 'user/register'))), 'error');
you can see that it's a false positive.

And as the review process is mainly about licensing and security we can't force applicants to follow standarts 100%
regarding the lines of code of this module, it's not surprising that there are so many errors flagged.

Good points, very interesting & thank you. I can test this mod if you think it is near release. Menu callback titles default to t(). from Drupal 6 unless redefining the t() callback function itself. True.

Another point, could use some documentation, and as said in above comments http://drupal.org/node/1399684#comment-5453002 by @klausi, still use a comparison to the Quiz module, it appears from the code and installing and running it to be a subset of the Quiz module. There appear to be a few different types of questions for this module, like the crossword compared to the quiz module, others are more similar.

Installing & testing it. Notes:
Needs documentation!

Was able to create some of the sample tests, like the crossword etc. Some of them worked well enough, and some others did not.

Error link: On page: admin/langtests/2 ( or admin/langtests/1 etc. the [Back to Tests] link is incorrect and gives: Not Found The requested URL /admin/langtests was not found on this server. and a link of admin/langtests?render=overlay. admin/langtests also does not work. Looks like /admin/langtests just put in as absolute path rather than relative.

Error link: On page: admin/langtests/testitem/edit/2 the Cancel button does the same thing as [Back to Tests] Above.

On a type the word test I got this error when trying to view the test:

Notice: Undefined offset: 0 in langtests_testtype_display() (line 325 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined index: in langtests_testtype_display() (line 325 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 0 in langtests_testtype_display() (line 342 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).

When I actually took my test of 3 questions I got the error above on the word test, and then at the very end received this:

Notice: Undefined offset: 0 in langtests_testtype_display_submitter() (line 603 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 1 in langtests_testtype_display_submitter() (line 603 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 2 in langtests_testtype_display_submitter() (line 603 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 3 in langtests_testtype_display_submitter() (line 603 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 4 in langtests_testtype_display_submitter() (line 603 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 0 in langtests_testtype_display() (line 408 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 0 in langtests_testtype_display() (line 410 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined index: in langtests_testtype_display() (line 410 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 0 in langtests_testtype_display() (line 413 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 1 in langtests_testtype_display() (line 408 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 1 in langtests_testtype_display() (line 410 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined index: in langtests_testtype_display() (line 410 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 1 in langtests_testtype_display() (line 413 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 2 in langtests_testtype_display() (line 408 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 2 in langtests_testtype_display() (line 410 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined index: in langtests_testtype_display() (line 410 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 2 in langtests_testtype_display() (line 413 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 3 in langtests_testtype_display() (line 408 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 3 in langtests_testtype_display() (line 410 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined index: in langtests_testtype_display() (line 410 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 3 in langtests_testtype_display() (line 413 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 4 in langtests_testtype_display() (line 408 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 4 in langtests_testtype_display() (line 410 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined index: in langtests_testtype_display() (line 410 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Notice: Undefined offset: 4 in langtests_testtype_display() (line 413 of /var/www/drupal7/sites/all/modules/language_tests/includes/testtype.ttw.inc).
Status message Congratulations! All your answers are correct!

Note: could keep testing the module, however, it is my belief that these probably should be looked at.

Also.
On code review I still get:
langtests.install

Line 385: Use st() instead of t() in hook_install(), hook_uninstall() and hook_update_N()
  $access_toolbar = t('Use the administration toolbar');
Line 386: Use st() instead of t() in hook_install(), hook_uninstall() and hook_update_N()
  $message = t("Don't forget to configure the module's permissions. You may wish to grant permission '@perm' to the 'teacher' role.", array('@perm' => $access_toolbar));

Reference: http://api.drupal.org/api/drupal/includes%21install.inc/function/st/7

testtype.wsr.inc

Line 138: use <br /> instead of <br>
$answers .= '<br>';

Could cause issues with markup validation. Still
*does* work and is standard for HTML5 working draft.

Line 195: in most cases, replace the string function with the drupal_ equivalent string functions
$prefix = substr($raw_task, 0, $pos);
From http://api.drupal.org/api/drupal/includes%21unicode.inc/function/drupal_...

Note that for cutting off a string at a known character/substring location, the usage of PHP's normal strpos/substr is safe and much faster.

@ecm there never were any, but darrellulm suggested to do so, that's the reason for my statement

Just want to state I crossed that statement out hours after I posted it in http://drupal.org/node/1399684#comment-5671444 so hope your browser is showing those issues crossed out like these words.

The larger concern of course are simply the run-time errors because of the implications of that type of error as shown in http://drupal.org/node/1399684#comment-5672690 which I am sure at this point you have investigated. I tested more in depth in http://drupal.org/node/1399684#comment-5672690 because I didn't want to let it stand with my earlier 15 minute lunch break test.

Also, I should say, the module is interesting and I do like the new and different types of quizes, such as the novel type of crossword test question. It is an ambitious module and has good aims, so keep up the good work.

please visit http://ventral.org/pareview/httpgitdrupalorgsandboxecm1398998git for the list of errors that are being generated.

Hi ecm!

Automated review looks great. Manual review:

• langtests.module:495 - Test type names are not themselves translated via t()
• The Libraries API at http://drupal.org/project/libraries is generally the recommended means of handling third-party libraries. This allows the library to exist outside of the module directory, eliminating the need to pull it back in every time the module gets updated. I personally wouldn't flag the review as "needs work" on these grounds alone, though I'd strongly recommend considering it.
• langtests-admin.inc:774 - Columns do not align when no items found; colspan should be 10.
• testtype.wrs.inc:211 - JS added via drupal_add_js is lengthy to the point of needing its own file for readability's sake. See the documentation on how to do this.
• testtype.wsr.inc:216 - Message should indicate that words cannot be selected on diagonals.
• Wordsearch admin page - Answers column only saving the first letter of each previous answer when adding words. Suspect it's in your script around testtype.wrs.inc:261.

Looks like a very ambitious project that's well on its way. Kudos!

Hi ecm,

In regards to your response to davidwatson, I quote:

I have to send translated strings into the script (see testtype.wrs.inc:216, 234, 237, 238, 323), therefore, it seems to me that it's impossible to put the script in a separate file.
Remained unchanged

I have rolled out a patch for you on what I believe davidwatson meant you to do.

Take a read at http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_a... to learn more of what drupal_add_js can do.

I did a manual review on your code and it looks good to me. Once you apply the patch I will RTBC this issue.

Check this link for the part of the errors
http://ventral.org/pareview/httpgitdrupalorgsandboxecm1398998git

wow, that is a LOT of code. you should definitely get a Review Bonus to get faster reviews.

well, i looked it up a bit, and here's my review:

One thing i noticed is that the admin can not edit or delete the other users' tests. I guess there should be more access levels and permissions throughout the code. Like, almost any other content or stuff has two levels of access, "edit own" and "edit any", "delete own" and "delete any", and so on; one for the normal author, and one for the admin user.

And, for every test created, there are two flags, "Enabled" and "Public", which i feel have the same purpose. What is their difference? why has this redundancy been created?

Also, right now, if the raphael-min.js file is not included in the libraries, this error is shown when starting a test:
Warning: file_get_contents(sites/all/libraries/raphael/raphael-min.js): failed to open stream: No such file or directory in _locale_parse_js_file() (line 1488 of /var/www/drupal/local/includes/locale.inc).
It would be more clear, if you checked for the js file before "drupal_add_js"-ing it, and showed a more meaningful error message, like "Your test need the raphael-min.js file, please include it according to Language Tests README." or something similar.

I'll try to give you another review as soon as i have the time.

Here's another review, as I promised:

You shouldn't use hook_init to add css or js files to your module.
Source: hook_init api docs

To add CSS or JS that should be present on all pages, modules should not implement this hook, but declare these files in their .info file.

another thing i noted, inside your hook_help code, is the use of urls in translatable texts. This is not good.
Because you can't change the url, without the translators having to translate the strings again.
The general rule is to keep the markup, but put the url as parameter, for example: t('<a href="@url">text</a>', ...)
also, fix strings like the t('Now you can create 9 types of tests:'); which means we have to translate the string, again, each time you add a new type of test; Add the number as a parameter to t().
For more information on using t(), check here:
http://drupal.org/node/322774

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Moved master -> 7.x-1.x-dev