This module adds to shs the ability to be used in search api views.

Project page:
https://www.drupal.org/sandbox/rigoucr/2324723

Git commands:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/rigoucr/2324723.git 
shs_search_api
cd shs_search_api

Manual reviews of other projects:
- https://www.drupal.org/node/2511680
- https://www.drupal.org/node/2485297
- https://www.drupal.org/node/2532260

Comments

rigoucr’s picture

Status: Active » Needs review
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/httpgitdrupalorgsandboxrigoucr2324723git

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.

kporras07’s picture

Status: Needs work » Needs review

Made some of the corrections suggested by robot.
The other corrections were not made because:

"Do not use the raw $form_state['input'], use $form_state['values'] instead where possible"
That was not corrected because there is not $form_state['values'] available.

Method names were not corrected because this class extends from a views filter handler.

Thanks for reviewing

darol100’s picture

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
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and the README Template. But I find that README.txt is very limited. It only few lines of codes without any useful information. I will recommend to follow this README.txt Template (https://www.drupal.org/node/2181737)
Code long/complex enough for review
Yes Follows the guidelines for project length and complexity.
Secure code
Yes

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

I have run your project on http://pareview.sh/pareview/ and I find few errors & warning.

You should consider fixing them http://pareview.sh/pareview/httpgitdrupalorgsandboxrigoucr2324723git

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).

    <p></p>
    <p>FILE: ..._temp/includes/handlers/shs_handler_filter_term_node_tid_search_api.inc<br>
    --------------------------------------------------------------------------------<br>
    FOUND 4 ERRORS AFFECTING 4 LINES<br>
    --------------------------------------------------------------------------------<br>
      18 | ERROR | Public method name<br>
         |       | "ShsSearchApiHandlerFilterTermNodeTid::option_definition" is not<br>
         |       | in lowerCamel format, it must not contain underscores<br>
      30 | ERROR | Public method name<br>
         |       | "ShsSearchApiHandlerFilterTermNodeTid::extra_options_form" is<br>
         |       | not in lowerCamel format, it must not contain underscores<br>
      56 | ERROR | Public method name<br>
         |       | "ShsSearchApiHandlerFilterTermNodeTid::value_form" is not in<br>
         |       | lowerCamel format, it must not contain underscores<br>
     176 | ERROR | Public method name<br>
         |       | "ShsSearchApiHandlerFilterTermNodeTid::admin_summary" is not in<br>
         |       | lowerCamel format, it must not contain underscores<br>
    --------------------------------------------------------------------------------</p>
    

    Time: 114ms; Memory: 7.25Mb<br>
    
  • DrupalPractice has found some issues with your code, but could be false positives.

    <p></p>
    <p>FILE: ..._temp/includes/handlers/shs_handler_filter_term_node_tid_search_api.inc<br>
    --------------------------------------------------------------------------------<br>
    FOUND 0 ERRORS AND 6 WARNINGS AFFECTING 5 LINES<br>
    --------------------------------------------------------------------------------<br>
      74 | WARNING | Do not use the raw $form_state['input'], use<br>
         |         | $form_state['values'] instead where possible<br>
      75 | WARNING | Do not use the raw $form_state['input'], use<br>
         |         | $form_state['values'] instead where possible<br>
      88 | WARNING | Unused variable $tid.<br>
     162 | WARNING | Do not use the raw $form_state['input'], use<br>
         |         | $form_state['values'] instead where possible<br>
     162 | WARNING | Do not use the raw $form_state['input'], use<br>
         |         | $form_state['values'] instead where possible<br>
     163 | WARNING | Do not use the raw $form_state['input'], use<br>
         |         | $form_state['values'] instead where possible<br>
    --------------------------------------------------------------------------------</p>
    

    Time: 52ms; Memory: 5.5Mb<br>
    
  • kporras07’s picture

    Thanks @darol100.

    About what you commented:

    I updated the project page. I think it looks better now.

    The errors reported by CoderSniffer are because that file extends from views_handler_filter_term_node_tid; so I need to name the functions on that way.

    The warnings reported by DrupalPractice are because in that point of the code I don't have those values in $form_state['values'].

    Thanks again,

    madhusudanmca’s picture

    Status: Needs review » Needs work

    Automated Review

    No. Please correct the issues reported at http://pareview.sh/pareview/httpgitdrupalorgsandboxrigoucr2324723git

    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 ,no master branch: 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: Follows the guidelines for in-project documentation and the README Template.
    Code long/complex enough for review
    No: Follows the guidelines for project length and complexity.
    Secure code
    Yes. If "no", list security issues identified.
    Coding style & Drupal API usage
    1. It's a very small module to promote as stand alone project. I suggest you to contact either SHS or Search API maintainers to add your code as an additional functionality (preferably with SHS)

    The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

    Moving this to needs work, I would like you to think again before promoting it as stand alone module, else I don't see any blockers for this.

    As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

    Thanks Again!

    PA robot’s picture

    Status: Needs work » Closed (won't fix)

    Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

    kporras07’s picture

    Status: Closed (won't fix) » Needs review

    Really sorry for being so late. I contacted the shs mantainer; but I never got answer from him. So; I'd expect to promote this as an standalone project; and I'd hope to get help for this.

    Thanks in advance

    naveenvalecha’s picture

    Assigned: Unassigned » naveenvalecha

    Assigning to myself for next review that might be in tonight.

    naveenvalecha’s picture

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

    First Thanks for your contribution!

    Automated Review

    There are lots of Best practice issues identified by pareview.sh / drupalcs / coder. http://pareview.sh/pareview/httpgitdrupalorgsandboxrigoucr2324723git

    Manual Review

    Individual user account
    Maybe: Follows the guidelines for individual user accounts.There are two authors of this projects.So do let us know which of the user wants the git vetted role.I see there are two committers in this project https://www.drupal.org/node/2324723/committers
    No duplication
    Yes: Does not cause module duplication and/or fragmentation.
    Master Branch
    Yes: Follows the guidelines for master branch.
    Licensing
    Yes: Follows the licensing requirements.
    3rd party assets/code
    Yes: Follows the guidelines for 3rd party assets/code.
    README.txt/README.md
    No: Does not follow 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: Meets the security requirements.
    Coding style & Drupal API usage
    1. A hook_help is missing in module.It would be nice it in the module.
    2. As the module is implementing the views filter views_handler_filter_term_node_tid so I would suggest to add the dependency on the views module.
    3. shs_handler_filter_term_node_tid_search_api.inc: You are adding js/css files using drupal_add_js/drupal_add_css rather use
      #attached to add the JS/CSS file
      with $form array.
    4. Just a recommendation

    The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

    If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

    This review uses the Project Application Review Template.

    I would recommend you, please help to review other project applications to get a review bonus back. This will put you on the high priority list, then you will come in admins radar :)

    PA robot’s picture

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

    Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

    kporras07’s picture

    Status: Closed (won't fix) » Needs review

    Hi there again;
    we had totally forgotten about this :(

    Taking this project again.

    Thanks @naveenvalecha; I already applied the changes that you suggested.

    I think this is ready to promote to full project; it would be great if someone could change status to RTBC and give full project status.

    Thanks in advance,

    kporras07’s picture

    Issue summary: View changes
    kporras07’s picture

    Issue summary: View changes
    Issue tags: +PAReview: review bonus
    klausi’s picture

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

    manual review:

    1. "module_exists('search_api')": you already depend on search_api_views which depends on search_api, so this check will always return TRUE and can be removed.
    2. shs_search_api_taxonomy_get_all_descendants(): doc block is confusing, this function does not return anything?
    3. shs_search_api_taxonomy_get_all_descendants(): why do you have to cast the tids to string? Please add a comment.
    4. README.txt is a bit short, see also https://www.drupal.org/node/2181737

    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.

    kporras07’s picture

    Thanks @klausi.

    About your comments:
    1) Removed
    2) Updated comment
    3) Removed unuseful cast and tested module functionality.
    4) Updated README.txt

    Really thanks for your review.

    heddn’s picture

    Status: Reviewed & tested by the community » Fixed

    In ShsSearchApiHandlerFilterTermNodeTid->value_form() one small nit. Integer indexed array isn't really necessary:

            '#attached' => array(
              'js' => array(
                0 => array(
                  'data' => $settings_js,
                  'type' => 'setting',
                ),
                1 => array(
                  'data' => drupal_get_path('module', 'shs') . '/js/shs.js',
                  'type' => 'file',
                ),
              ),
            ),

    Just nest these as array elements directly.

    This is not even close to a blocker. Thanks for your contribution, Rodrigo! Maybe I'll even see you in a week at Drupal Camp CR?

    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.

    rigoucr’s picture

    Thanks @heddn :D, I apply your suggestion and of course I'll see you en Drupal Camp Costa Rica :D

    Status: Fixed » Closed (fixed)

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