This module extends the module Taxonomy Term Permissions by providing four Rules actions: (1) Grant User a Term Permission, (2) Revoke a User's Term Permission, (3) Grant a Role a Term Permission, and (4) Revoke a Role's Term Permission, which are the four main actions that TTP provides.

The module gets a clean pass from Coder.

Sandbox: https://drupal.org/sandbox/sprice/2041485
Drupal-Git: git clone --branch 7.x-1.0 git.drupal.org:sandbox/sprice/2041485.git term_permissions_rules_actions

some reviews I've done
---
#2030421: [D7] Flag Notify
#2036083: [D7] Multiple Fields Remove Button
#2041597: [D7] Unveil
https://drupal.org/node/2030421#comment-7645399
https://drupal.org/node/2036083#comment-7645381
https://drupal.org/node/2041597#comment-7644031

Comments

PA robot’s picture

Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxsprice2041485git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

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

sprice’s picture

Fixed everything reported by the Project Applications Scraper (latest: http://ventral.org/pareview/httpgitdrupalorgsandboxsprice2041485git) except for the naming of the branches. I'll fix that shortly.

sprice’s picture

(deleted -- double post)

sprice’s picture

Status: Needs work » Needs review

Changing the status back to needs review.

feolius’s picture

Status: Needs review » Needs work

Hi!

I'm not an expert but I met few things that probably need to be fixed.

function term_permissions_rules_actions_add_role($term, $role) {

  if (!(term_permissions_rules_actions_check_existence($user, 'uid'))) {
    // Check to see if the user exists.
    drupal_set_message(t('No user exists with that uid (@user)',
      array('@user' => $user)), 'error');
   ...
  }
function term_permissions_rules_actions_remove_role($term, $role) {

  if (!(term_permissions_rules_actions_check_existence($user, 'uid'))) {
    // Check to see if the user exists.
    drupal_set_message(t('No user exists with that uid (@user)',
      array('@user' => $user)), 'error');
  }

Maybe I miss something, but I don't see any declarations of $user variable. Maybe you forget to add global $user string?

Why do we need this code

if ($user) {
    $table = 'term_permissions_user';
    $type = 'uid';
    $obj = $user;
  }
  elseif ($role) {
    $table = 'term_permissions_role';
    $type = 'rid';
    $obj = $role;
  }

  $result = db_select($table, 't')
    ->fields('t', array($type))
    ->condition('tid', $term)
    ->condition($type, $obj)
    ->execute()
    ->rowCount();

I don't see any using of $result after this.

$id = db_insert('term_permissions_role')
        ->fields(array(
          'tid' => $term,
          'rid' => $role,
        ))
        ->execute();

$id is not used too, but as I understand we really need this code. So, you could just get rid from $id here.

Sorry, if I understand something incorrect.
Thanks.

sprice’s picture

Status: Needs review » Needs work

(duplicate)

sprice’s picture

Status: Needs work » Needs review

You're right. I was accidentally working from an earlier file (I'm still getting used to branching in git), so the $user variables were supposed to be $role variables. I fixed those.

I got rid of the result and id variables. They were there for extra checks earlier, but rules didn't like those checks (and the code already does enough before that to make sure everything is valid and exists).

The code

if ($user) {
    $table = 'term_permissions_user';
    $type = 'uid';
    $obj = $user;
  }
  elseif ($role) {
    $table = 'term_permissions_role';
    $type = 'rid';
    $obj = $role;
  }

is used so that I could write one function instead of two in order to do a query to make sure that the permission for the term with the user/role doesn't already exist so we don't get a SQL error for a duplicate entry. The TTP module sets up two tables (the two in the above code) that have just two columns (tid, uid / tid, rid). So I need to make sure that the permission isn't already in the table (neither use an id of their own, so there won't be duplicate entries). Since the function is called for either user or role, then I just made it so that it checked for only one variable. So the only difference between the queries is the table . So, I just wrote one function instead of two. Does that make sense? It works, but there might be a more appropriate way to do the same thing with Drupal best practices. If there is one, then let me know, and I'll change the code.

Thanks for the review. Everything is fixed.

sprice’s picture

Status: Needs work » Needs review
Issue tags: +PAreview: review bonus

Update
---
reviewed a few modules, so also tagging this issue with the "review bonus program."

https://drupal.org/node/2030421#comment-7645399
https://drupal.org/node/2036083#comment-7645381
https://drupal.org/node/2041597#comment-7644031

sprice’s picture

Status: Needs review » Needs work

Further testing exposed a few bugs. I'll fix them before moving it back to needs review.

sprice’s picture

Status: Needs work » Needs review

Bugs fixed. Version also passes PAReview with no warnings or errors.

Please review and comment! Thanks.

feolius’s picture

Status: Needs review » Needs work

Thanks for your response.

But I still don't understand this codeblock. The problem is that it just makes a select from table (and it is ok), but you don't use the result of that select somehow and there is no "return" operator after this. This code will be executed, but it gives no influence on the result of the check.

Thanks.

sprice’s picture

Status: Needs work » Needs review

Now I get what you're saying. Leaving it out also led to a failed check from time to time, causing a database error. So, I put it back in.

I set it back so that the select captures the variable. Then if the select returned any rows, it returns true, letting the function know that the permission already exists in the table and to set a warning but do nothing further.

Here's the change:

  $result = db_select($table, 't')
    ->fields('t', array($type))
    ->condition('tid', $term)
    ->condition($type, $obj)
    ->execute()
    ->rowCount();

  if ($result) {
    return TRUE;
  }
  else {
    return FALSE;
  }

Thanks for the quick response.

klausi’s picture

Status: Needs review » Postponed (maintainer needs more info)

This sounds like a feature that should live in the existing term_permissions project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the term_permissions issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

klausi’s picture

Oh and please add all your reviews to the issue summary, so that we can track them.

klausi’s picture

Issue summary: View changes

changed git to newer branch

sprice’s picture

Issue summary: View changes

added some reviews

sprice’s picture

I've seen quite a few rules modules that extend other projects, so I figured that it would be normal to create the module as a separate project. I was planning on contacting the maintainers of the project to see if they wanted to link to this one on the description page.

But I did go ahead and open an issue in their queue (#2042579: Rules Functionality), and I added the reviews up top.

feolius’s picture

Thanks!

Much better now:)

PA robot’s picture

Status: Postponed (maintainer needs more info) » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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

PA robot’s picture

Issue summary: View changes

added reviews again