The Nodereference filter module will add a new admin section to manage node autocomplete filtering options per nodereference auto complete field, per node type, useful is using the same nodereference field across multiple node types. For exampe node_parent field, this module would enable the ability to set specific node types to be a parent of any given node type.
This module will overwrite the ajax request and give the ability to apply filtering options when searching for a node in the autocomplete field.

LINK: http://drupal.org/sandbox/PsycleInteractive/1407660
Repository: git.drupal.org:sandbox/PsycleInteractive/1407660.git
Drupal Version: 6

CommentFileSizeAuthor
#5 Node reference filter.txt952 bytescamdarley

Comments

b2f’s picture

b2f commented

I wish you luck to get a review, in the meantime you can check the automated review generated by ventral :
http://ventral.org/pareview/httpgitdrupalorgsandboxpsycleinteractive1407...
Even if this is only automated and full of coding styles errors, it could help you to be taken seriously here, I think.
Regards.

misc’s picture

misc commented
Status: Needs review » Needs work
Psycle Interactive’s picture

Psycle Interactive commented

@Zen master, thank for the helpful advice. I have reviewed the code and made the appropriate changes to resolve the issues raised the tool you provided.

Thanks again for the helpful tip.

misc’s picture

misc commented
Status: Needs work » Needs review
camdarley’s picture

camdarley commented
Status: Needs review » Needs work
StatusFileSize
new Node reference filter.txt952 bytes

I PsycleInteractive,

It appear that your branch name is wrong, It should be 6.x-1.0 and not 6.x-1.0-dev.
You can read the documentation about release naming conventions if you need more informations.

Also, I checked your code trough ventral.org and there is still errors in node_autocomplete_edit_link.css.
You can use the automated review process yourself to be sure your project is ready to review.

Regards.

Psycle Interactive’s picture

Psycle Interactive commented

Thanks for the feedback camdarley.
I have resolved the code issues highlight through ventral, i have created the branch as a dev branch at the moment because that is what it it. Once the module has been approved and removed from sandbox i will then create the 6.x-1.0 release branch, as at that point the module is accepted and ready for release.

Psycle Interactive’s picture

Psycle Interactive commented
Status: Needs work » Needs review

Have updated the branch to be called 6.x-1.x, all release tags in the 6.x-1.x range will be created from this base branch.
Once the module is accepted and removed from sandbox mode i will create the first tag 6.x-1.0, i am reluctant to tag it now incase code changes are needed in order to get the module accepted.

misc’s picture

misc commented

There are a module called Nodereference filters, it does different things, but you should look into a name that differs more.

prashantgoel’s picture

prashantgoel commented
Status: Needs review » Needs work

please visit http://ventral.org/pareview/httpgitdrupalorgsandboxpsycleinteractive1407... for the list of errors being generated

patrickd’s picture

patrickd commented
Status: Needs work » Needs review

don't block deeper reviews because of minor issues found by automated reviews

novalnet’s picture

novalnet commented

Hi,
Manual Review :

1. function node_autocomplete_edit_link_form_alter(&$form, &$form_state, $form_id) should be prefixed with your module/theme name.
2. You should follow alphabetical order for Multiple CSS properties in node_autocomplete_edit_link.css.
3. You should PARREVIEW your script. You have many errors.
See http://git.drupal.org/sandbox/PsycleInteractive/1407660.git

Psycle Interactive’s picture

Psycle Interactive commented

OK, i have resolved the code styling issues pointed out by the automated review.
Note that the function node_autocomplete_edit_link is within a sub module called node_autocomplete_edit_link, therefore the name of the function is correct for the module. I don't know why the automated review does not pick this up.

crobinson’s picture

crobinson commented
Status: Needs review » Needs work

This is probably because you can hook_form_alter() and you can also hook_FORM_ID_form_alter and I don't know that the automated tool can handle this complex of a name. Changing the name of the module so it doesn't end in edit_link (edit is a common element in a form ID) might fix this, but it's not necessary and it's extremely unlikely that this will lead to a conflict down the road.

A few comments from manual review:
1. In your .install module for node_reference_filter you delete variables directly without using variable_del(). I can see why because you can create wildcard variables and it could be tedious to manually find them all and remove them one-by-one. But if you do it this way you're bypassing the cache_clear_all('variables', 'cache')/unset() that variable_del() also performs. As an alternative a good pattern is listed here: http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/varia.... This is important for sites that use memcache, particularly with clustered nodes, because the variables persist in memcache until cleared and a re-install of the module (if the admin is trying to re-install, say, to fix a problem or "start over") (module_disable() and drupal_uninstall_module() don't clear cache).

2. I was very confused by the README.txt - it took me half an hour to get the example working, not because of any problem but just because I didn't understand how the examples under step 2 really worked. Could you provide one or two more examples just to flesh this out?

3. This module is obviously only useful if nodereference is installed and enabled - but it's not listed as a dependency in the .info file.

4. On line 59 of node_reference_filter.admin.inc you have a PHP parse error - you need an extra ')' to close the t() call.

5. This is JUST a suggestion, but there are numerous spots where direct database queries are made where an API call would do as well. For instance, on line 62 of node_reference_filter.admin.inc, you query node_type in a way that could equally well be served by content_get_types('type', $type_name). I don't think this is something that should fail a review, I'm just making the suggestion because it's capable of using the cache so it's faster if memcache is enabled (really nice for an AJAX call) and also because it'll make an eventual Drupal 7 port easier.

6. Is there a reason node_autocomplete_edit_link.js doesn't use a behavior? The way it's written it looks to me like on multiple-cardinality fields it has no way of triggering on newly-added rows. Unless I've configured something wrong on my system?

Psycle Interactive’s picture

Psycle Interactive commented
Priority: Normal » Major
Status: Needs work » Needs review

Hi crobinson,

Thank you for your detailed response. I have gone through each point and made any amends or will highlight my thoughts in response:

1) I have updated the code to look for variables in the $conf global that are for my module and perform a variable_del on that, this removes the need to query the database directly.

2) Not sure what else could be added, the example are there to show you what you can entered into the autocomplete field to apply the filtering options when searching for a node.

3) I have added the dependancy on the nodereference module, thanks for pointing this out.

4) I don't get this php error in my git checkout of the code, there are 3 open and 3 close brackets on the line

5) I tried using the function you mentioned to get content types (content_get_types), but this caused an undefined function error. so i have left the code as it is. Note that i have already created the drupal 7 branch for this module

6) This only needs to be added to autocomplete fields that have a value entered in them at the time the page it loaded, any changes will not be shown until the node is saved. That is why i only do it on a document ready.

Thanks again for looking into the module.

crobinson’s picture

crobinson commented
Status: Needs review » Needs work

OK, I think I know what happened. You have files in your master branch. You need to clear out the files there per the instructions here: http://drupal.org/node/1127732.

Specifically, remove the files from master and make the README.txt described there. Some of the other instructions aren't relevant until you have a full project status.

It is the files in your master branch that have the above mentioned error.

I see that you created the Drupal 7 branch, but did not perform code review on that branch. I focused on the D6 branch. The purpose of this process is to review authors and their coding skills / adherence to standards, not to review modules, so as soon as you make the final change described above I will sign off on this on the basis of the review that I did there. Thanks for your contribution!

Psycle Interactive’s picture

Psycle Interactive commented
Status: Needs work » Needs review

Thanks for the information, i have cleared all the files out of the master branch and left a README file pointing developers to the version branches.

Both the D6 and D7 branches have been through code review several times, so i don;t mind if you want to review the D7 before signing off. Both are in a state ready for release.

crobinson’s picture

crobinson commented
Status: Needs review » Reviewed & tested by the community

Looks good, moving to next step.

misc’s picture

misc commented

Just some minor warnings:

FOUND 0 ERROR(S) AND 5 WARNING(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
 73 | WARNING | A comma should follow the last multiline array item. Found:
    |         | 'Filter Options'
 79 | WARNING | A comma should follow the last multiline array item. Found:
    |         | 'node_reference_filter_help'
 85 | WARNING | A comma should follow the last multiline array item. Found:
    |         | '</div>'
 93 | WARNING | A comma should follow the last multiline array item. Found:
    |         | 'Create a node of a content type related to this field.'
 97 | WARNING | A comma should follow the last multiline array item. Found:
    |         | 'node_reference_filter_create_help'

Else, thanks for your contribution, PsycleInteractive! 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.

Thanks to the dedicated reviewer(s) as well.

misc’s picture

misc commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.