[D7] Ajax Document Viewer

Hi Mohammad,

Thanks for your contribution!

I have found some issues that you should review for getting the project approved:

• Use 7.x-1.x branch instead of master for development. Check Release naming conventions
• The coder module reports some minor issues. Here they are:
  

  sites/all/modules/ajax_document_viewer/ajaxdocumentviewer.module:
  +4: [minor] There should be no trailing spaces
  +5: [minor] There should be no trailing spaces
  +12: [minor] Comment should be read "Implements hook_foo()."
  +26: [minor] Comment should be read "Implements hook_foo()."
  +38: [minor] There should be no trailing spaces
  +45: [minor] There should be no trailing spaces
  +52: [minor] There should be no trailing spaces
  +59: [minor] There should be no trailing spaces
  +66: [minor] There should be no trailing spaces
  +68: [minor] There should be no trailing spaces
  +69: [minor] There should be no trailing spaces
  +74: [minor] There should be no trailing spaces
  +76: [minor] There should be no trailing spaces
  +77: [minor] There should be no trailing spaces
  +82: [minor] There should be no trailing spaces
  +84: [minor] There should be no trailing spaces
  +85: [minor] There should be no trailing spaces
  +86: [minor] There should be no trailing spaces
  +90: [minor] There should be no trailing spaces
  +92: [minor] There should be no trailing spaces
  +93: [minor] There should be no trailing spaces
  +97: [minor] There should be no trailing spaces
  +99: [minor] There should be no trailing spaces
  +100: [minor] There should be no trailing spaces
  +101: [minor] There should be no trailing spaces
  +109: [minor] Comment should be read "Implements hook_foo()."
  +122: [minor] There should be no trailing spaces
  +129: [minor] Comment should be read "Implements hook_foo()."
  +159: [minor] There should be no trailing spaces
  +173: [minor] Comment should be read "Implements hook_foo()."
  +175: [minor] There should be no trailing spaces
  +180: [minor] Use an indent of 2 spaces, with no tabs
  +183: [minor] Use an indent of 2 spaces, with no tabs
  +193: [minor] l() already contains a check_plain() call by default. See http://drupal.org/node/28984
  +204: [minor] l() already contains a check_plain() call by default. See http://drupal.org/node/28984
  +215: [minor] There should be no trailing spaces

• The README should wrap at 80 characters. See the documentation guidelines.
• Using function names without the prefix is a bad idea, because any other module could overwrite it accidentally, or you could override some other module functions. Change get_url method name to _ajaxdocumentviewer_get_url.
• If I have understood well how this works, this lets the Ajax Document Viewer view your documents. This should be clear in the homepage of your module.
• I'm not sure about how this will work if the admin enables private urls. If the files settings must be public urls, this should be clear on the homepage of your module.

Please, mark this issue as 'Needs review' once this issues are fixed.
Thanks again for your contribution!

Hi asifnoor,

Review of master branch:

• Create the branch for 7.x-1.x

Coder review

• Line no 21 description of menu should not be enclosed in t().

Some suggestions :

• There are typo errors : it should be "Implements" instead of "Implements of" in the comments
• Form validation comment can be articulated in a different way instead of "Implements hook_form_alter()"
• Line No 9 & 250 variable AJAX_URL should be AJAXDOCUMENTVIEWER_URL
• Line No 252, instead of hardcoding file system path. You may user file_create_url().
• You may include the validation for global image file location.
• Line no 64, hexadecimal input in default_value is not valid.
• In README.txt, correct url for config [line no 28] should be admin/config/user-interface/ajaxdocumentviewer

Please find the attached patch file.

Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/ajaxdocumentviewer.module:
   +5: [minor] There should be no trailing spaces
   +6: [minor] There should be no trailing spaces
   +8: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 2 files, 3 minor warnings, 0 warnings were flagged to be ignored
  

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• ./ajaxdocumentviewer.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function ajaxdocumentviewer_settings_form() {
  --
  
  function ajaxdocumentviewer_settings_form_validate(&$elements, &$form_state, $form_id = NULL) {
  --
  
  function ajaxdocumentviewer_field_formatter_info() {
  --
  
  function ajaxdocumentviewer_theme() {
  --
  
  function theme_ajax_document_view_new_window($items = NULL) {
  --
  
  function theme_ajax_document_view_lightbox($items = NULL) {
  --
  
  function theme_ajax_document_view_inline($items = NULL) {
  --
  
  function _ajaxdocumentviewer_get_url($items) {
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• "'access arguments' => array('access administration pages'),": do not abuse that permission, create your own.
• ajaxdocumentviewer_settings_form(): there should be no space between function and doc block. Also on other functions.
• ajaxdocumentviewer_settings_form_validate(): indentation error in the last if
• ajaxdocumentviewer_field_formatter_view(): the "break;" should be indented one level further, see http://drupal.org/node/318#controlstruct . The last foreach() should be indented one level further.
• theme_ajax_document_view_inline(): all user facing text should run through t()
• "* The Ajax Document viewer module enables you to": comments should break at 80 characters, not earlier.

Hi,

automatic review ends up in only one more issue:

Review of the 7.x-1.x branch:

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual review:

• You should clean up the master branch as described in http://drupal.org/node/1127732 step 5

Looks almost ok for me.

Some more issues show after installing the most recent review tools:

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ...ites/all/modules/pareview_temp/test_candidate/ajaxdocumentviewer.module
  --------------------------------------------------------------------------------
  FOUND 3 ERROR(S) AND 3 WARNING(S) AFFECTING 6 LINE(S)
  --------------------------------------------------------------------------------
    10 | ERROR   | Space after closing parenthesis of function call prohibited
   155 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
   160 | ERROR   | Closing brace indented incorrectly; expected 4 spaces, found 2
   206 | WARNING | A comma should follow the last multiline array item. Found: )
   209 | WARNING | A comma should follow the last multiline array item. Found: )
   212 | WARNING | A comma should follow the last multiline array item. Found: )
  --------------------------------------------------------------------------------
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./ajaxdocumentviewer.info ./README.txt ./js/ajaxdocumentviewer.js ./ajaxdocumentviewer.module
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

hth, dave

You can check your module online: http://ventral.org/pareview :)

I ran parview.sh and found a few errors: mostly due to one thing in your doxygen documentation. If you put a space after your summary and or description in your doxygen comments most of these errors will go away. If you look at http://drupal.org/node/1354 it shows how to properly space your doxygen comments. Doxygen documentation is generated by a computer and to parse the documentation the spacing and everything else has to be laid out just right ...

FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
75 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...ites/all/modules/pareview_temp/test_candidate/ajaxdocumentviewer.module
--------------------------------------------------------------------------------
FOUND 20 ERROR(S) AFFECTING 8 LINE(S)
--------------------------------------------------------------------------------
219 | ERROR | There must be an empty line before the parameter block
219 | ERROR | Last parameter comment requires a blank newline after it
219 | ERROR | Missing parameter type at position 1
221 | ERROR | Data type of return value is missing
232 | ERROR | There must be an empty line before the parameter block
232 | ERROR | Last parameter comment requires a blank newline after it
232 | ERROR | Expected 1 space before variable type
232 | ERROR | Doc comment for var elements does not match actual variable name
| | $items at position 1
232 | ERROR | Missing comment for param "elements" at position 1
234 | ERROR | Data type of return value is missing
248 | ERROR | There must be an empty line before the parameter block
248 | ERROR | Last parameter comment requires a blank newline after it
248 | ERROR | Expected 1 space before variable type
248 | ERROR | Doc comment for var elements does not match actual variable name
| | $items at position 1
248 | ERROR | Missing comment for param "elements" at position 1
250 | ERROR | Data type of return value is missing
265 | ERROR | There must be an empty line before the parameter block
265 | ERROR | Last parameter comment requires a blank newline after it
265 | ERROR | Missing parameter type at position 1
267 | ERROR | Data type of return value is missing

I read through your javascript which isn't long and just notice one minor thing:

CamelCasing
Unlike the variables and functions defined in Drupal's PHP, multi-word variables and functions in JavaScript should be lowerCamelCased. The first letter of each variable or function should be lowercase, while the first letter of subsequent words should be capitalized. There should be no underscores between the words.

You have a js variables which doesn't use the camel casing: var div_id = "#document-viewer-" + fid;

I just installed your module, got a free key from the external site. Then plugged in the api key and hit save taking the other values as defaults and receive the following error:

Color should be in Hexa decimal values only

the toolbar color is the problem and you have it default to: EE000

It looks to me like you meant the default to have one more zero to be EE0000 .... when I plug in the xtra zero it saves without an error.

Ok so I have my api key and followed your readme and I'm not sure how to proceed based on the directions. Do I somehow get an account at http://www.ajaxdocumentviewer.com/ and upload my docs there and then construct my links in drupal like so: http://connect.ajaxdocumentviewer.com?key=&document=&viewerheight=&viewerwidth=

I think the readme needs to be updated. I am unable to figure out what to do next. I think users will give up after 15 minutes. I don't have time to figure out what to do, but I that leads me to suggest that you add to your README.txt file with more complete info on how to setup this module from start to finish, so a user can be up and running within 15 minutes.

Ok I dug a little deeper and saw that if I put the link on the page <a href='http://connect.ajaxdocumentviewer.com?key=<key>&document=http://www.irs.gov/pub/irs-pdf/fw4.pdf&viewerheight=768&viewerwidth=1024'>test</a>

the link will open the document on the http://connect1.ajaxdocumentviewer.com/ site. This worked with your module installed. But then I went and tested it without your module installed on another drupal site. It also worked! So I'm not sure what this module does then, since it worked with and without your module installed. I'm sure I'm missing something. I see you implement hook_field_formatter_view so maybe this modifies cck in some way so a link will be outputted with a link structured automatically like: <a href='http://connect.ajaxdocumentviewer.com?key=<key>&document=http://www.irs.gov/pub/irs-pdf/fw4.pdf&viewerheight=768&viewerwidth=1024'>test</a>

Anyhow an end Drupal user wont have any idea what to do to get this working. I don't have any more time to dive deeper. I'll just wait for the README to get updated -- that is my recommendation.

Cheers,

Barnettech

The _ajaxdocumentviewer_get_url() function immediately screams that it is possibly vulnerable to XSS because it injects raw variables into the URL and the result of _ajaxdocumentviewer_get_url() is never run through check_url() in theme_ajax_document_view_inline(). The other calls are run through l() which applies check_plain().

function theme_ajax_document_view_inline($items = NULL) {
  drupal_add_js(drupal_get_path('module', 'ajaxdocumentviewer') . '/js/ajaxdocumentviewer.js');
  $file_url = _ajaxdocumentviewer_get_url($items);
  $output = l($items['items']['filename'], '', array('attributes' => array('class' => 'show_iframe', 'fid' => $items['items']['fid'])));
  $output .= "<div class = 'ajaxdocumentview-iframe' id = 'document-viewer-" . $items['items']['fid'] . "' style = 'display: none; clear: both;'>";
  $output .= "<iframe name='ajaxdocumentviewer' src=" . $file_url . "border='0' height=" . variable_get('ajaxdocumentviewer_viewer_height', '500') . " width=" . variable_get('ajaxdocumentviewer_viewer_width', '500') . " scrolling='no' align='left' frameborder='0' marginwidth='1 marginheight='1'>" . t("Your browser does not support inline frames or is currently configured not to display inline frames.") . "</iframe>";
  $output .= "</div>";
  return $output;
}

Note that $file_url is output without being escaped first. The variables ajaxdocumentviewer_viewer_height and ajaxdocumentviewer_viewer_width are also the same, although it does look like these are validated, it is still a best practice to escape them. Make sure to review the handbook page on handling text in a secure fashion on how to fix this vulnerability properly.

Issue priority should only be raised by having no review after x weeks. (See review workflow)
If you want to get "high priority" by review bonus you have to tag your issue with "PAReview: review bonus" instead.

Did a manual review and tested the module...works just fine!!

No reviews listed in the issue summary, so removing review bonus tag. See #1410826: [META] Review bonus

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

manual review:

• theme_ajax_document_view_new_window(): $file_url has been ran through check_url() which calls check_plain(). The l() function also calls check_plain(), so you are doing double escaping here which is bad. You should do sanitization as closely as possible to the actual output to avoid that (i.e. when you embed the variable into HTML or print it otherwise). So you should remove check_url() from _ajaxdocumentviewer_get_url() and use it only in theme_ajax_document_view_inline() where you generate raw HTML.
• _ajaxdocumentviewer_get_url(): $base_url is never used.

But that are just minor issues, so ...

Thanks for your contribution, asifnoor! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Just can't this to work, already got a free key and everything seems to be ok and I always get a the message: "loading" and it doesn't load.

Explain explain how to use it, already read the readme file and no conclusion. I installed the module, got a free key, made a new file field and choose Ajax document lightbox view. What am I doing wrong?

please check:

http://connect.ajaxdocumentviewer.com/?key=K80068113112012144134&documen...

or

http://learn4fun.no-ip.org/node/49 and click in the pdf file

Thanks for helping!

Adding review of other projects information