GroupDocs Word, Powerpoint, Excel and Acrobat PDF Viewer Embedder Module

Looks good so far!

The only real issue I have here is that this title is extremely long for such a small module!

Unfortunately project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project.

As the installation instruction are quite short, there's also no need to put them into an extra file. readme would fit good here too.

regards

a small issue there please check and fix http://ventral.org/pareview/httpgitdrupalorgsandboxgroupdocs1699856git

Well the current version is by no means small, it's now HUGE.

However, some of that bulk is the inclusion of third-party libraries (fancybox and jqueryFileTree) as part of this module (wrong, put them in /libraries if they are really needed) and are entirely incompatible with GPL : http://www.fancyapps.com/fancybox/#license - so totally wrong there.
If you want a modal popup (?) please use one of the half-dozen others that are already supported as Drupal library modules, don't bring in your own that includes its own copy of jquery.

When I did a git pull, it asked for submodules - which is odd.

Need a note on the project page that the 3rd-party API you are using is non-free, requires pricing and an API key to work.

There is a lot in this module, and a lot that should be improved.

js/groupdocs.js - don't use the plain jquery onload event to trigger your jquery additions, use Drupal.behaviours and attach: instead. This eliminates conflicts with ajax loads, overlays, and cases where page DOMs change dynamically.
js/treeviewerpage.js - same as above. Code style there will probably need work to improve it for - eg - multiple fields, or more than one upload field per page. If that's out of scope, take some steps or at least add a warning that things will break if you use two of these fields on one node.
/templates - why are you using an html attribute <div class="" name="groupdocsBrowser"> ? Do you mean the (actually valid) id="" instead? Not a code review error, but possibly an issue to improve.

Actually getting to the code - include_once(dirname(__FILE__) . '/lib/groupdocs-php/api/APIClient.php');
no. Should be using module_load_include() please.

    echo "<script>
      window.parent.document.getElementById('edit-field-groupdocs-und-0-groupdocs-embedded-code').value = '" . @$result->result->guid . "';
      window.parent.jQuery.fancybox.close();
    </script>"; die;

this is just scary.

groupdocs_viewer_upload_file_page($form) {    
    drupal_static_reset();
    
    $path = drupal_get_path('module', 'groupdocs_viewer');
    drupal_add_js($path . '/lib/fancybox/lib/jquery-1.7.2.min.js', 'file');
  ...
  ...

Is also scary in several ways.
In a form context, unfortunately you can't reliably use drupal_add_js() like that. Try using #attached instead. This makes a difference when using modals, caching, or when a form has validation issues.

function groupdocs_viewer_js_alter(&$js) {
  unset($js['misc/jquery.js']);
  unset($js['misc/jquery.once.js']);
  unset($js['misc/drupal.js']);
  unset($js['misc/jquery.cookie.js']);
  unset($js['modules/toolbar/toolbar.js']);
}

O_o ... really?

    print("<ul class=\"jqueryFileTree\" style=\"display: ;\">");
    foreach ($folders as $item) {
            print("<li class=\"directory collapsed\"><a href=\"#\" rel=\"" .
                      $path . $item->name . "/\">" . $item->name . "</a></li>");
    }

o_O ... there are theme functions for this.

OK. I'm going to stop looking now.

Sorry, needs a lot of work. Right now it's not clean or sane enough to pass.

1. "GroupDocs Word, Excel, Powerpoint, PDF Viewer Embedder lets you embed several types of files into your WordPress pages "... shouldn't it say Drupal not Wordpress?! ;)

2. In readme it's sayd that gdoc_field module must be enabled. Is this optional as it's not dependency in .info file?

3. Shouldn't configuration be under admin? groupdocs_viewer/config vs admin/groupdocs_viewer/config

4. Under groupdocs_viewer/config form item descriptions should describe the field in more detail and not be equal to the label.

5. I haven't yet configured the module and probably that's the reason I'm getting fatal error @ groupdocs_viewer/treeviewer

Notice: Undefined index: dir in groupdocs_viewer_treeviewer_page() (line 98 of /home/devel/hannes/sites/all/modules/contrib/groupdocs_viewer/groupdocs_viewer.admin.inc).
Notice: Undefined property: stdClass::$message in APIClient->callAPI() (line 128 of /home/devel/hannes/sites/all/modules/contrib/groupdocs_viewer/lib/groupdocs-php/api/APIClient.php).
Exception: Unauthorized API request to https://api.groupdocs.com/v2.0/storage/{userId}/folders/?page=0&count=10&order_by=&order_asc=false&filter=&file_types=&extended=false: in APIClient->callAPI() (line 127 of /home/devel/hannes/sites/all/modules/contrib/groupdocs_viewer/lib/groupdocs-php/api/APIClient.php).

I think fatal error should not occur.

6. Yep. Error is gone after setting the fields. Now I'm getting listing of 3 links inside ul list without rest of the Drupal HTML:

<div class="item-list"><ul class="jqueryFileTree"><li class="file ext_pdf first"><a href="/hannes/%23" rel="08d32ee2558180b9fbdb947e061603546aeb68732edd08a1b96ba1da568c9bbd" class="iframe">Quick_Start_Guide_To_Using_GroupDocs.pdf</a></li>
<li class="file ext_pdf"><a href="/hannes/%23" rel="cb23731b856877f9276dae95fb6c103cc9c033f6904002307138acf0ee0b6b9e" class="iframe">Quick_Start_Guide_To_Using_GroupDocs_Document_Assembly.pdf</a></li>
<li class="file ext_pdf last"><a href="/hannes/%23" rel="4cdd37221c343d08838c2d465954d0479582785a41f4cb3154db7fade6ad31d1" class="iframe">Quick_Start_Guide_To_Using_GroupDocs_Signature.pdf</a></li>
</ul></div>

7. I would even add "Field UI" as a dependendy because I had it disabled but the readme told me to 'Click on "manage fields" under Home » Administration » Structure'

8. When I tried to upload, it opened a popup and sayd:

 Not Found

The requested URL /groupdocs_viewer/upload_file/lightbox2 was not found on this server.

Add applications reviewed

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

• Drupal Code Sniffer has found some issues with your code (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. You have to get a review bonus to get a review from me.

manual review:

1. Please note: All user accounts are for individuals. Accounts created for more than one user will be blocked when discovered. Please change your account/username settings accordingly.
2. groupdocs_viewer_field_schema(): no need to copy parts of the doc block from hook_field_schema(). Just your own comments please.
3. "'access arguments' => array('access administration pages'),": The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.
4. groupdocs_viewer_config_form(): this is not a hook and should not be documented as such. See http://drupal.org/node/1354#forms
5. "variable_get('groupdocs_client_key')": all variables used by your module must be removed in hook_uninstall().
6. groupdocs_viewer_field_widget_form(): "Choose file" is user facing text and must run through t() for translation.
7. groupdocs_viewer_field_widget_form(): the link will not work if drupal is installed in a subdirectory. Better use l() or similar functions to generate link markup.
8. groupdocs_viewer_uploading_form_submit(): do not use exit(), use drupal_exit() instead.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

I reviewed problems in comment #8. All seems ok

Here're some new questions:

1. Why is there groupdocs_field in readme.txt while the project itself is groupdocs_viewer? That and the first line in Readme - "groupdocs_field is an extension to the core field module, which must be enabled." - would imply that there's some other modules that need to be enabled before groupdocs_viewer?

2. I uploaded a pdf. It showed up in the tree, clicked on it and saved the node. But the document viewer says "The document is not found." Using latest chrome, delete browser and Drupal cache.

Good job, it works for me now!

1. Ok, though it still seems strange sentence. I'd replace it whith short description what this module is all about. One probably has field module enabled anyways. Use something from you project page!

2. About this section:
Unpack the groupdocs_viewer folder and contents in the appropriate modules
directory of your Drupal installation. This is probably
sites/modules/
... There are too many spaces before "This is probably" ( I know I'm pedantic :) ) and sites/modules/ should be sites/all/modules

3. groupdocs_viewer/treeviewer should be type MENU_CALLBACK. It appeas in my admin menu.

4. $form['#attributes']['enctype'] = 'multipart/form-data'; is not needed in D7.

5. I'm not sure why my admin menu is in region-page-bottom but that's probably reason why I see it in upload page. I think safe would be to create your own html template and include the form in there.

6. I think there shouldn't be dot in the "Upload file or enter embedded code." because I don't see any other widget labels using full stops. Check it out when you add new fields.

7. I'd restrict file upload for only documents that your reader can understand. You can do it by creating _validate method - groupdocs_viewer_uploading_form_validate($form, &$form_state) {

8. Libraries should be under sites/all/libraries Why don't you fork the changed project under github and link it from there?

I agree those are feature/improvement suggestions, not code review blockers.
This has gone through enough cleanup to be +1 now IMO.

Thanks for your contribution, groupdocs!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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.

Thanks to the dedicated reviewer(s) as well.