easyrec for Ubercart

ZIP files in #1 are corrupt - can't review.

just downloaded and installed it....works fine for now - waiting for first real results now.

@segments: You downloaded it from where? The .rar files referenced in the OP? A hex dump of the .zip files attached to #1 shows they aren't zip files at all ...

Bottom line, a proposed project needs to be posted here in the forum, not as a link to some other site.

@TR I had them from here...but probably I was able to open them because I use winrar.

@david.mann Thx for the support.

You need to run the Coder module to bring your code up to Drupal coding standards: http://drupal.org/coding-standards

The parameters "configure" and "files[]" aren't valid in a Drupal 6.x .info file.

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

Your project structure is a little messed up.

You have three branches: master, 6.x-dev, and 7.x-dev.

First, your development branches should be named 6.x-1.x and 7.x-1.x, that way drupal.org can automatically create development snapshots for your project. Second, if you're not going to use master (and most people don't), you should not have anything in master except for perhaps a README.txt explaining that you have to checkout one of the named topic branches to get the code. Deleting master is not an option because by default git clone will check out the master branch. When following the clone instructions at the top of your sandbox page, for example, it gets master by default, which doesn't contain anything meaningful.

Second, your folders look like this:

easyrec_for_ubercart/            <-- Why is everything in this directory?
  easyrec_for_ubercart.info      <-- What is this?
  easyrec/                       <-- Why isn't this the top-level directory?
    easyrec.admin.inc
    easyrec.info
    easyrec.install
    easyrec.js
    easyrec.module

I see you're missing a project README.txt in the easyrec/ directory. Also, you still really need to read all of http://drupal.org/coding-standards and try to make your code comply with that as much as possible. You still have a ways to go ... In particular, your indents are wrong and inconsistent (sometimes you use 4, sometimes you use 2, sometimes you mix 4 and 3 and 2...) You also have a lot of trailing spaces and lines with nothing but spaces on them, all throughout all of your code. They should be removed. It's hard to read and evaluate your code when it looks like this.

You have dynamically-named variables that need to be removed in hook_uninstall().

I think David got the idea about how Drupal modules are handled and this project has made a good progress so it can go into production as a Drupal module. :)

updating title

Some feedback. I only reviewed the 7.x-1.x version, some things also apply to 6.x-1.x, others don't.

files[] = easyrec.module
files[] = easyrec.admin.inc
files[] = easyrec.install

version = 7.x-1.0

- Only add files to files[] when they contain classes (for example test or views integration classes).
- Do not add a version, that will automatically be added when a release is created by the packaging script

/**
 * Implementation of hook_requirements().
 */
function easyrec_requirements($phase) {

The coding standard for Drupal 7 suggests the shorter "Implements hook_something()." now. Also, other hook implementations in the .install file are missing the docblock completely. Please add a similar docblock to all hook implementations

function easyrec_permission() {
    return array(
        'administer  easyrec' => array(
            'title' => t('Administer easyrec'),
            'description' => t('Perform maintenance tasks for easyrec.'),
        ),
    );
}

There are two spaces in the permission name. But there aren't when you use that permission for example in hook_menu().

    $items['admin/config/system/easyrec'] = array(

The system category isn't really the correct place for this. I would suggest Web Services (services). D7 only.

        if (arg(0) == 'node' && is_numeric(arg(1))) {
            $node = node_load(arg(1));

- I suggest you use "if ($node = menu_get_object())" instead of this.

/**
 * Implements hook_block_info().
 */
function easyrec_block_info() {
    $blocks['otherUsersAlsoViewed']['info'] = t('easyrec: other users also viewed');
    $blocks['otherUsersAlsoBought']['info'] = t('easyrec: other users also bought');
    $blocks['recommendationsForUser']['info'] = t('easyrec: recommendations for user');

    $blocks['mostViewedItems']['info'] = t('easyrec: most viewed items');
    $blocks['mostBoughtItems']['info'] = t('easyrec: most bought items');

    return $blocks;
}

I think you should at least set the cache property too, see http://api.drupal.org/api/drupal/modules--block--block.api.php/function/.... It defaults to cache per role which is probably not what you want.

            '#size' => 45,
            '#maxlength' => 45,
            '#description' => 'An parameter to determine the number of results returned.',
        );

The description isn't translated here. Same for all other block configure form elements I think, maybe others too. Make sure all text strings can be translated.

            if ($node->type == 'product' || $delta == "recommendationsForUser") {

I think there is a uc_is_prodct() function or something like that. AFAIK, you can have many product node types, not just a single one.

- From what I see, the admin settings allow to configure the javascript used, which obviously means that one should be very careful with giving that permission away. In D7, you can set 'restrict access' in hook_permission to true, which will display a warning to users. See http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...

One more.

function easyrec_admin_settings_form_validate($form, &$form_state) {
    // Trim some text values.
    $form_state['values']['easyrec_tenantId'] = trim($form_state['values']['easyrec_tenantId']);
    $form_state['values']['easyrec_apiKey'] = trim($form_state['values']['easyrec_apiKey']);
    $form_state['values']['easyrec_url'] = trim($form_state['values']['easyrec_url']);
    $form_state['values']['easyrec_drawDrupalCustom'] = trim($form_state['values']['easyrec_drawDrupalCustom']);

}

You should use form_set_value() instead of directly overwriting form_state['values']-

http://api.ubercart.org/api/function/uc_product_is_product/2

This is the function I mean for checking the node type.

+    $blocks['otherUsersAlsoViewed'] = array('info' => t('easyrec: other users also viewed'),
+                                            'cache' => DRUPAL_NO_CACHE);

Should look more like this:

$blocks['otherUsersAlsoViewed'] = array(
  'info' => t('easyrec: other users also viewed'),
  'cache' => DRUPAL_NO_CACHE,
);

         if (arg(0) == 'node' && is_numeric(arg(1))) {
-            $node = node_load(arg(1));
+            $node = menu_get_object();

Not what I meant :)

You just need:

if ($node = menu_get_object()) {

That's the same as your code. There is also a similar one in hook_page_alter().

- * @return
- *   String containing a Drawback alias
+ * @return  String containing a Drawback alias

The old code was actually correct here, it should be on a new line, intended with two spaces and ending with a .

+/**
+ * This function returns a Javascript string which sends a buy action to
+ * the easyrec server.
+ * @param  $order  the order object to get the product information
+ * @return string  the easyrec javascript
+ */

Same here, should be like this:

/**
 * This function returns a Javascript string which sends a buy action to
 * the easyrec server.
 * @param $order
 *   The order object to get the product information.
 * @return
 *   The easyrec javascript.
 */

Ok, the code looks good to me now (note that I only reviewed the D7 part, but I saw that 6.x has been updated too where appropriate and they shouldn't be too different). So from a technical point, this is RTBC I think.

However, I haven't used or even installed the module, so it would be great if someone could do that and try it out. If everything works as it should, this can be marked as RTBC. It might be rather hard to properly test this module because I assume that you need a real site with data, so that easyrec can do it's job.

However, a quick check to verify that it can be installed and configured properly should be enough I guess.

I don't think it should be a requirement of these applications that the project be actually installed and tested by a human - if the code passes visual code review, then the maintainer ought to be able to make it work. Manual testing is just extra work for a process that is a huge bottleneck.

So I'm just going to mark this RTBC.

OK, Git vetted user role granted.

Thanks for your present and future contributions and for your patience during this process!

Please review others' applications so we can get this queue down.