Closed (won't fix)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
23 Feb 2015 at 12:20 UTC
Updated:
13 Dec 2015 at 21:16 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
nabil.sadki commentedAutomated review
Git errors:
Review of the 7.x-1.x branch (commit 69e7058):
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.
Source: http://pareview.sh/ - PAReview.sh online service
Comment #2
marabak commentedAll errors are now fixed.
Comment #3
arnaud.dabouis commentedHi, I did a manual review.
1- file crossword_generator.module, line 34
The label is not translatable
2- file crossword_generator.module, line 92
The implemented hook is not mentioned in the comment.
3- file crossword_generator.info, line2
The description should begin with an uppercase letter.
Except for these 3 errors, everything looks great in terms of functionality and code standards.
Comment #4
marabak commented1 & 3 fixed.
2 : crossword_generator_crossword_field_process() is a process function for crossword_field element not a hook.
Comment #5
arnaud.dabouis commentedOk for 1 and 3. My bad for 2, you are right.
Comment #6
mlmoseley commentedIndividual User Account
Yes
Duplication
Yes.
Master Branch
Yes
Licensing
Yes
3rd Party Assets
Yes
README.txt/README.md
Text is insufficiently explanatory and uses incorrect terminology. The module folder is not a 'webform directory'. The phrase 'Create a new content of type "Crossword" is incorrect english. It should be 'Create a new node of the content type "Crossword". Also the the instructions seem to dictate that the content type be named 'Crossword', when that is not necessary.
Readme does not note that you need to specify a multi value field to get multiple words in the puzzle.
Secure Code
Yes
Coding Style
Seems fine.
Comment #7
marabak commentedAbout the README.txt.
Comment #8
zbombicz commentedHi marabak,
Automated Review
Minor pareview warnings:
FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
21 | WARNING | Line exceeds 80 characters; contains 93 characters
28 | WARNING | Line exceeds 80 characters; contains 82 characters
----------------------------------------------------------------------
Manual Review
The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.
If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.
This review uses the Project Application Review Template.
Comment #9
PA robot commentedClosing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #10
marabak commentedFollowing zbombicz recommendations:
Coding style & Drupal API usage:
1 - no more theme for crossword_field and no more warnings
2 - more restrictive css rules : all prefixed width .field-type-crossword-field
3 - hook_help() implemented
4 - crossword_generator_field_formatter_info() cleaned
5 - crossword_generator.js cleaned
Comment #11
PA robot commentedThere are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxmarabak2430325git
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #12
marabak commentedall preview errors and warnings were handled
Comment #13
blanca.esqueda commentedHi,
I loved the idea, maybe later on as improvement a review button can be added to check the words on the crossword.
Testing your module I found out that sometimes the following message is displayed:
crossword:
Failed to find crossword.
That makes sense when the words doesn't have a matching letter. But I tested with a few words (image attached), and even after refreshing the page 10times didn't get the crossword.
Comment #14
marabak commentedok i'will check it - so i change the status to "needs work"
your suggestion is good - i think i will change the crossword to a drupal form with a validation process and a submit button that will check if the crossword is properly filled
Comment #15
marabak commentedThe issue is due to the third party code that i'm using..
A workaround is to change the order of the words. I'm still on it :)
Comment #16
PA robot commentedClosing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).
I'm a robot and this is an automated message from Project Applications Scraper.