Tefiltro

Hi rodrigo,

Please add a git clone link to the issue.

It seems you are working in a master branch. Please fix that, you can find a description here.

Ventral found a bunch of other issues, please fix them.

It will speed up the review process, if you get a Review bonus.

Do not assign the issue to yourself, this is meant for the one doing an in depth review.

Hi,

Posted at the same time that @musikpirats. I edit to avoid duplication.

@1002Studio // thanks!
please don't paste these clumsy automated reviews in here, please edit you last comment and short it
@rodrigo please, switch to a version specific branch before switching back to needs review

Manual Review :-

• tefiltro.module:- Line 66, 72, 78 You are setting variables but not deleting them anywhere. Add a .install file and delete them in hook_uninstall()
• tefiltro.module:- Line 63, 69, 75 Why are you adding $user->uid to form elements name. Is there any specific reason and is it not possible to name them without a uid ?

Added git clone

Manual review:
You are mixing languages in your code:

  return array(
    'administer tefiltro' => array(
      'title' => t('Administer Tefiltro'),
      'description' => t('Tefiltro settings'),
      'restrict access' => TRUE,
    ),

function tefiltro_form_validate($form, &$form_state) {
  $table_name = $form_state['values']['tefiltro_vocabulary'];
  if (!$table_name) {
    form_set_error('tefiltro_vocabulary', t('Você deve digitar o nome de algum vocabulário.'));
  }
  elseif (!db_table_exists('field_data_field_' . $table_name)) {
    form_set_error('tefiltro_vocabulary', t('A tabela não existe.'));
  }
}

There are some mixed english and spanish comments too. Please rewrite the comments in english, so other non-spanish people can understand your module and help commiting.

  $amigos_array = array();
  // Pega o node do grupo!
  // Gets group node!
  $gnids = tefiltro_dbgnode($user->uid);
  // Pega o organic group id do node!
  // Get og id of node!

There's still portuguese comments, comments and variable names should be in English, and use US English spelling. See http://drupal.org/node/1354#general.

Removing review bonus tag, you did not do any actual manual reviews, you just pointed out some missing git links. Please see http://drupal.org/node/894256

Rodrigo,

Please check out the README file again because I think you misspelled the node type name "drive" on the item 6 of the step-by-step section.

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

Manual Review:

1) Please check spelling in the documentation of tefiltro_getfriends() function.
2) Remove unnecessary comments from tefiltro_filtersfilter() fucntion.

note that many of the issues found are minor and should not be required for approval, therefore please do not insist on having them fixed and do not switch the issue to needs work if there are no major issues found.

tefiltro.module

• line 121: variable 'friends' might have not been defined
• line 212: you defined $term_array = array(); but then you're using $terms_array[] = $term->tid;.
• line 218: you defined $node_array = array(); but then you're using $nodes_array[] = $node->nid;

I set status to 'needs work' because if these errors with variable name declaration and access...

Just a detail, change you git clone command in the issue description to
git clone http://git.drupal.org/sandbox/panchiniak/1521136.git tefiltro

Updated: description.

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

Hi,

First off, git clone http://git.drupal.org/sandbox/panchiniak/1521136.git tefiltro ends up with a tefiltro directory with a README.txt in it that says "See major version branches". It only works properly if you add --branch 7.x-1.x.

Having said that, below are my comments:

I think you need to spell tefiltro as Tefiltro in tefiltro.info. You should also do that for use in hook_permission implementation in tefiltro.module (so that it wouldn't be the last permission on the permissions page).

You don't need 'type' => MENU_NORMAL_ITEM in hook_menu implementation.

You might be better off renaming tefiltro_form to tefiltro_settings_form for clarification.

Updated informal presentation link.

A few comments:

Repository

You seem to have an extra '1521136' submodule entry in your git repository, viewable at http://drupalcode.org/sandbox/panchiniak/1521136.git/tree/refs/heads/7.x... ... that should be removed.

tefiltro.module

Line 7: define('TEFILTRO_VERSION', '7.x-1.x-dev');
You define a constant here, but it is never used in your code. It should probably be removed. The Drupal.org packaging scripts will add a version line to the tefiltro.info file to help differentiate between versions.

Line 88: elseif (!db_table_exists('field_data_field_' . $table_name))
Use the Field Info API to determine if a field exists. In this case, you likely want to check whether field_info_field($tefiltro_vocabulary) returns a value.

Line 139: $result_getnodes = $query_getnodes ...

Line 275: function tefiltro_dbgnode($uid) { ...
The two queries need to have ->addTag('node_access') added to them, to respect node access permissions on the site. See http://drupal.org/node/310077 for more usage information.

Line 161: function tefiltro_viewtid($tid, $nid) { ...

Line 183: function tefiltro_getterm($nid) { ...

Line 339: function tefiltro_dbfiltro($nid) { ...
The queries in these function should probably be rewritten to leverage EntityFieldQuery ... see http://drupal.org/node/1343708 for usage examples.

Line 279: ->condition('n.type', 'group', '=')
Is the node 'type' guaranteed to be 'group'? I think EntityFieldQuery might be more flexible here ... but I'm not certain which version of OG you're developing against, so I'm not certain.

From what I read in README.txt, noderefcreate is a dependency; so you should put in in telfitro.info, in dependencies section.

1- My first statement on http://drupal.org/node/1522456#comment-7032772 still stands, you should update the default branch from master to 7.x-1.x, or delete master branch.

2- Edit .info file, and replace 'correspondent' with 'corresponding'.

Automatic review
You should remove the master branch. I see you already set 7.x-1.x as the default branch, but the master branch still exists.

Manual review
PAReview reports two errors in tefiltro.module: the break statement in line 28 is unnecessary because it comes after a return statement. Also there is an extra line after that break that should be removed. But since there is only one case I think it's better to use an if construct instead of a switch, like this:

function tefiltro_help($path, $arg) {
  if ($path == 'admin/help#tefiltro') {
      return '<p>' . t('Tefiltro filters term reference fields by the authorship of the corresponding nodes') . '</p>';
  }
}

Note that I also switched to single quotes as they are recommended in the coding standards (also note that I wouldn't bring that up on its own, but since we were on that line...)

You should leave an empty line between functions.

The function tefiltro_settings_form is not an implementation of hook_form, it's a callback you define for the settings page. You should correct the comment.

I'm not sure this module needs to exist, sorry. I understand the idea - that you can limit term reference fields to only show options that belong to a user's nodes. But the proper way to handle this is to use a view to display the term reference fields (or any reference field). The view contains the logic you're asking for (only tags applied to my nodes or those of my friends).

Sorry if I'm not understanding the purpose - is there a reason this cannot be done with views?

Marking needs work based on previous comment.

----
Top Shelf Modules - Enterprise modules from the community for the community.

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

I'm a robot and this is an automated message from Project Applications Scraper.

Added branch at git clone.