Project Summary

Right now there is no module available to setup nodequeue for node reference.

This module allows users to create a node reference sub-queue from the selected content types to have a unique sub-queue. You can place nodes into any of these sub-queues based on which node reference that node has been tagged with.

You can select a node reference field and associated node type to create sub queue for each node-reference item.
This module is an add-on of the Nodequeue module and uses Nodequeue APIs to create node reference sub queues.

Requirements:

1. You must install the latest version of Nodequeue.module
2. You must install the latest version of NodeReference.module

Sandbox URL
https://www.drupal.org/sandbox/icreonglobal/2162181

Git Repository
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/IcreonGlobal/2162181.git node_reference_subqueue
Pareview :
http://pareview.sh/pareview/httpgitdrupalorgsandboxIcreonGlobal2162181git
Reviews:
https://www.drupal.org/node/2325487
https://www.drupal.org/node/2326343
https://www.drupal.org/node/2310391
https://www.drupal.org/node/2316681
https://www.drupal.org/node/2327875

CommentFileSizeAuthor
Node-Reference-Subqueue.jpg329.09 KBIcreonGlobal
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

IcreonGlobal’s picture

Status: Active » Needs review
PA robot’s picture

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.

IcreonGlobal’s picture

I have already verified this module through http://pareview.sh/ and coder module.

I would appreciate everyone feedback on this module.

pkamerakodi’s picture

Hi,

Thanks for the contribution, actually i am really interested in using this function is some of my works. I would spend time testing and playing around it with. Mean while some small suggestions if you like to update.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
Yes

Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements
3rd party code
Yes: Follows the guidelines for 3rd party code.
README.txt/README.md
Yes
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
YES
Coding style & Drupal API usage
1) '#description' => t('What to display for the subqueue title; use %subqueue to embed the actual subqueue title. This is used to distinguish multiple nodequeues with subqueues from each other, as internal subqueue title is filled automatically.'),

Does not seem like u r passing value for %subqueue

2) hook_help is missing

3) Try using _field_sql_storage_tablename instead of using
$join_table = 'field_data_' . $field_name;

4) hook_nodequeue_form,hook_nodequeue_form_validate i didnot see this hook defined in
http://drupalcontrib.org/api/drupal/contributions!nodequeue!nodequeue.mo...

5) I think the below code will be little heavy if possible try using entity query or db_select becuase u just need title and moreover it is getting node_reference_subqueue_nodequeue_subqueue_title is called in a two for loop
foreach ($nids as $nid) {
// If node exists.
if ($nid) {
$ref_node = node_load($nid);
$titles[$nid] = $ref_node->title;
}
}

6) Sorry little confused on this node_reference_subqueue_build_string i think below is not going to get executed becuase $arrays is empty you are existing the recursion
$strings = array();

foreach ($array as $term) {
foreach ($substrings as $string) {
$strings[] = "$term-$string";
}
}

7) I think it should implements hook_form_alter then
Implements hooknode_reference_subqueue_form_alter().

8) Comment is misleading it isn't for function node_reference_subqueue_subqueue_form_submit

Implements hook_node_reference_subqueue_subqueue_form_submit().form submit is not a hook it is custom form submit callback right.

9) Add !empty check before calling nodequeue_load and nodequeue_load_subqueue in node_reference_subqueue_subqueue_form_submit

10) Small suggestion may be you could better format this line
$is_exist = db_select('nodequeue_nodes', 'nqn')->fields('nqn', array('nid'))->condition('nqn.qid', $qid)->condition('nqn.sqid', $sqid)->condition('nqn.nid', $nid)->execute()->rowCount();

Prajwal

pkamerakodi’s picture

Status: Needs review » Needs work
IcreonGlobal’s picture

@pkamerakodi, thanks for your reviews.

I will implement all your reviews and update status.

IcreonGlobal’s picture

I have fixed all above reviews and updated git repo. here is my comments for each.

#1, fixed

#2, fixed

#3, in my case not getting field structure object to pass in this function. so i managed this in such way.

#4, fixed respective comments

#5, fixed

#6, fixed

#7, fixed

#8, now comment is updated.

#9, fixed

#10, fixed.

@pkamerakodi, once again thanks for all your suggestions.

IcreonGlobal’s picture

Status: Needs work » Needs review
gaurav.pahuja’s picture

Can you please let me know the usage of 'hook_info' in your module?

gaurav.pahuja’s picture

There is no need to define 'default' statement in hook_help. Simply return $output.

$output .= '<p>' . t('This module allows users to create a node reference sub-queue from the selected content types to have a unique sub-queue.') . '</p>';
return $output;

Line # 167

$title should be initialized before the for loop.

Why do you have $queue variable added as first parameter inside function node_reference_subqueue_nodequeue_subqueue_title?

Line #229. Do we need to have a dependency on i18n module?

gaurav.pahuja’s picture

Status: Needs review » Needs work
IcreonGlobal’s picture

@gautav.pahuja Thanks for your time and feedback.

I have updated help text to have some more information about module usages so i think it should be there.

For other 2 points, i have updated module file.

There is no dependency on i18n module.

IcreonGlobal’s picture

Status: Needs work » Needs review
IcreonGlobal’s picture

This module is ready for further approval process. Can someone take a look.

pgautam’s picture

Status: Needs review » Needs work

1) Can you please define "global $user" at the start of the function instead of in between "foreach loop".
on line 281 in node_reference_subqueue.module file.
2) Its a suggestion please use db_query instead of db_select in the function "node_reference_subqueue_subqueue_form_submit" on line 343 please find below link specifically showing performance comparison between db_query and db_select.
https://www.drupal.org/node/1067802
"db_select" basically fires extra hooks within itself which are not required in your case i.e why i suggest you to use db_query.

IcreonGlobal’s picture

@pgautham, thanks for your all suggestions. We have fixed both issues.

#1, we have updated $global variable position.

#2, this is also fixed.

IcreonGlobal’s picture

Priority: Normal » Critical
Status: Needs work » Needs review

We have fixed all reported issues.

We have couple of other modules ready in sandbox for approval including some new interesting modules. As we can submit only one project application at a time so currently rest of our modules are on hold.

So i would request @drupal.org projects mentoring team to proceed this application for further approval so that we can also contribute our rest of modules at drupal.org ASAP.

IcreonGlobal’s picture

Priority: Critical » Major
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://pareview.sh/pareview/httpgitdrupalorgsandboxIcreonGlobal2162181git

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

IcreonGlobal’s picture

Status: Needs work » Needs review

We have fixed PAReview issues.

naveenvalecha’s picture

Issue summary: View changes

Updated issue summary.

naveenvalecha’s picture

Issue summary: View changes

@IcreonGlobal
Thanks for your contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder are fixed. http://pareview.sh/pareview/httpgitdrupalorgsandboxIcreonGlobal2162181git

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements
3rd party code
No: Follows the guidelines for 3rd party code.
README.txt/README.md
No: Follows the guidelines for in-project documentation and the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes. If "no", list security issues identified.
Coding style & Drupal API usage
The coding seems fine to me.

Thanks again !

This review uses the Project Application Review Template.

naveenvalecha’s picture

Priority: Major » Normal

Changed issue priority to Normal.There are 4 applications in the needs review with pareview bonus . Your application will be soon reviewed by the admins.

klausi’s picture

Assigned: Unassigned » heddn
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

Review of the 7.x-1.x branch (commit e4fabac):

  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. so this module does not work entity reference fields? Please add a sentence about that to README and project page, entity reference seems to be the more generic and popular solution.
  2. node_reference_subqueue_nodequeue_info(): doc block is wrong, hook_info() does not exist.
  3. node_reference_subqueue_nodequeue_form(): doc block is wrong, this is not hook_form().
  4. node_reference_subqueue_node_delete(): you should use named placeholders for the db_query() to avoid ordering errors, see the placeholders at https://www.drupal.org/node/310072

But otherwise looks good to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to heddn as he might have time to take a final look at this.

IcreonGlobal’s picture

Thanks @naveenvalecha and @klausi for your reviews.

In this release, we have not written any test cases for this module.

Here is my feedback for other points.

1- Added the note related to Node Reference Subqueue support for type of entity.

2- Updated doc block for node_reference_subqueue_nodequeue_info.
Ref: http://www.computerminds.co.uk/nodequeues-developers

3- Updated doc block for node_reference_subqueue_nodequeue_form().
Ref: http://www.computerminds.co.uk/nodequeues-developers

4- Added placeholders in db_query inside the function node_reference_subqueue_node_delete():

Now can you please take a look and approve this module so that we can also cross check our others modules and promote at drupal.org

heddn’s picture

  • In hook_help: Don't put urls into translated strings because it makes translation more difficult. Use variable replacement:
    t('From the Nodequeue interface (admin/structure/nodequeue/add/node_reference_subqueue), select a node reference field, define queue size and other values then select respective content type(s )to create a node reference queue.')
  • Spelling of atleast.
    t('Please create atleast one Node Reference Field.')
  • Missing @return tag.
    node_reference_subqueue_nodequeue_subqueue_title

However, none of these are blockers... so thanks for your contribution, Nikki!

I updated your account so you can 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:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay 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.

IcreonGlobal’s picture

Thanks @heddn for your review and feedback.

Also, Thanks to all reviewers for their great reviews and time spent on this project.

er.pushpinderrana’s picture

Assigned: heddn » Unassigned
Status: Reviewed & tested by the community » Fixed

Project approved, marking as fixed :).

Status: Fixed » Closed (fixed)

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