[D7] LinkedIn Company API Suite

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2313463

Project 2: https://www.drupal.org/node/2313465

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

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.

@Michael Hodge Jr, first of all thanks to you for contribution.

Automated Review

PAReview(http://pareview.sh/pareview/httpgitdrupalorgsandboxmichaelhodgejr2310985git) doesn't reported any issue in current build. Its good.

Manual Review

1. You have added linkedin-company-shares.css using linkedin_company_shares_init() that include this css on every page of site. I would recommend you to add this css using #attached, it would help you to add it specifically.

2. Your module is little bit complex but missing hook_help() in both .module file. It would be better if you add this.

3. In one scenario I am getting following notice, please fix it.

Notice: Undefined index: error in linkedin_company_access_token() (line 179 of C:\xampp\htdocs\drupal730\sites\all\modules\2310985\linkedin_company.module).

Rest looks good to me. Your project page and README.txt is well organised.

Good work @Michael Hodge Jr!

Automated Review

PAReview(http://pareview.sh/pareview/httpgitdrupalorgsandboxmichaelhodgejr2310985git) doesn't reported any issue.

Manual Review

Code appears good to me.

I spent some time looking through your documentation and found the following issue.

Under Synopsis you note that there are actually two modules in the project. You then have LinkedIn Company bolded, and discuss that module. In the next paragraph it looks like you are discussing the second module but have it titled LinkedIn Company again, but it is not bolded. This is confusing and you should consider revision.

Other than that everything seems well thought out and put together.

Great it looks like the issues are fixed.

astrocling, the RTBC status should be set by someone like er.pushpinderrana after he has confirmed those changes - you're not supposed to do that. Cheers.

#13 is a bit confusing ... this isn't astrocling's application, and they reviewed in #9, so there should be no problem with them marking it RTBC.

I haven't reviewed myself, but resetting the status as per #12.

#14 I recognise it's not astrocling's application but he had made only two posts prior to that one and I felt he wasn't in a position to accept er.pushpinderrana (#6)'s review on behalf of er.pushpinderrana. I also read the "status" requirements which, aside from being fairly loose, outline this exactly (that an experienced developer should be the one changing the status).

As you (jthorson) appear to be a regular contributor & community member, I'll leave the status as is and trust you've changed it for the right reasons.

Thanks Matt.

Given the amount of traffic which comes through this queue, and the fact that this process is driven entirely by volunteers, we like to encourage others to participate in reviewing as much as possible ... so our policy is that "anyone can review an application", and by extension, anyone can adjust the status of an application accordingly. If something is missed before an application is set to RTBC, it is the role of the git admins to catch that during a final review before promoting the project.

Thank you to both of you for helping contribute to the on-boarding of new contributors, and thank you Michael for your code contribution!

I've confirmed that each of the items in #6 have been addressed.

@Jthorson thank you for the kind words. I appreciate that the Drupal community strives to be open to participation from anyone.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

(*) Maybe: Does not cause module duplication and fragmentation. Can you follow the steps in the link and confirm that the functionality contained in this module should stay separate from linkedin?

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: Follows the guidelines for in-project documentation and the README Template. However, there is some inconsistencies in the documentation. It mentions there are two modules then calls both modules the same name.

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

1. (*) The project short name linkedin_company_api_suite does not match any of the the modules. At least one of the modules should match.
2. (+) I get highly annoyed by hook_init messages. I understand the reason for it, but it would be great to have a switch to turn it off, especially for local development when I might be using an older DB dump. Maybe a variable_get() check should be in order.
3. hook_init: Don't use time(), rather use the constant REQUEST_TIME
4. (*) hook_help: the inconsistencies in the README are duplicated in the hook help. You also might follow some of the more recent guidelines for hook_help and format as a mark down file and save yourself some work.
5. In general, arrays shouldn't be whitespace segmented.
   Incorrect:

     $form['linkedin_company'] = array(
       '#title'       => t('LinkedIn Company Integration'),
       '#type'        => 'fieldset',
       '#collapsible' => TRUE,
       '#collapsed'   => TRUE,
     );

   Correct:

       $form['linkedin_company'] = array(
       '#title' => t('LinkedIn Company Integration'),
       '#type' => 'fieldset',
       '#collapsible' => TRUE,
       '#collapsed' => TRUE,
     );

6. This is formatted incorrectly. The new arrays should start on the previous lines. Example to follow.
   

     $form['linkedin_company']['linkedin_company_company_id'] = array(
       '#default_value' => variable_get('linkedin_company_company_id', ''),
       '#description'   => t('You can use the !lookup_tool to easily determine your company id.',
         array(
           '!lookup_tool' => l(t('Company Lookup Tool'), 'https://developer.linkedin.com/apply-getting-started#company-lookup',
               array(
                 'attributes' =>
                 array(
                   'target' => '_blank',
                 ),
               )
           ),
         )
       ),

   Correct:

       '#description'   => t('You can use the !lookup_tool to easily determine your company id.', array(
           '!lookup_tool' => l(t('Company Lookup Tool'), 'https://developer.linkedin.com/apply-getting-started#company-lookup', array(
               'attributes' => array(
                 'target' => '_blank',
               ),
             )
           ),
         )
       ),

7. (+) I'm not sure if there is a requirement to use md5(rand()) but drupal_get_token or drupal_random_key is a better solution that is salted.
8. company.module lines 239-247: Use url() rather than building the URL manually. I also see on line 255-263
   

       $params = array(
         'response_type=code',
         'client_id=' . $consumer_key,
         'state=' . $random,
         'scope=rw_company_admin',
         'redirect_uri=' . urlencode(url(NULL, array('absolute' => TRUE)) . 'linkedin/company/token?step=authorized'),
       );
   
       $url = LINKEDIN_COMPANY_OAUTH2_API_BASE_URL . 'authorization?' . implode('&', $params);

9. Why doesn't linkedin_company_form_linkedin_admin_validate actually, well, validate the values? That functionality seems to exist in the access_token callback.
10. (*) Getting a wall of errors when placing the block without fully configuring the shares module.
    

    Warning: simplexml_load_string(): Entity: line 79: parser error : Opening and ending tag mismatch: meta line 5 and head in linkedin_company_shares_get_data() (line 121 of sites/all/modules/linkedin_company_api_suite/linkedin_company_shares/linkedin_company_shares.module).
    Warning: simplexml_load_string():  in linkedin_company_shares_get_data() (line 121 of sites/all/modules/linkedin_company_api_suite/linkedin_company_shares/linkedin_company_shares.module).
    Warning: simplexml_load_string(): ^ in linkedin_company_shares_get_data() (line 121 of sites/all/modules/linkedin_company_api_suite/linkedin_company_shares/linkedin_company_shares.module).
    Warning: simplexml_load_string(): Entity: line 84: parser error : Opening and ending tag mismatch: img line 84 and a in linkedin_company_shares_get_data() (line 121 of sites/all/modules/linkedin_company_api_suite/linkedin_company_shares/linkedin_company_shares.module).
    Warning: simplexml_load_string(): u/img/logos/logo_linkedin_119x32.png" width="119" height="32" alt="LinkedIn">

11. (*) The hook_requirements for simplexml should move into the submodule. It isn't needed except with company shares.

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.

1. Could you open an issue to Linkedin and confirm that they don't have plans to incorporate similar functionality any time soon?
2. Short name: I got that shortname from: https://www.drupal.org/project/2310985/git-instructions. This will also become part of the url when it becomes promoted to a full project so it really should be changed now.
3. I don't see where a call to validate the API key is valid, only that it exists. Maybe I'm missing something.

Thanks for your patience in the process.

Hmm, me thinks that the short name (#2) is something that gets configure at time of project promotion. So ignore for now.

Reference to the issue in linkedin module queue #2316721: LInkedin Company API Intentions, with my answer as one of the maintainer (summary: go for it !)

Marking RTBC again. Assigning to jthorson for a final review (if he has time).

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

• 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. linkedin_company_init(): this will run on every single page request for every user, is this really necessary? It will even run on drush invocations. I think you should rather send out an email in hook_cron() or display that on the admin page, but not in hook_init().
2. linkedin_company_form_linkedin_admin_validate(): instead of checking if $company_id is set here you should just make the form element #required in linkedin_company_form_linkedin_admin_alter(). Same in linkedin_company_shares_form_linkedin_admin_validate() with $cache_length.
3. linkedin_company_form_linkedin_admin_alter(): doc block is wrong, this is hook_form_FORM_ID_alter().
4. linkedin_company_authorize(): instead of checking $form_state['values']['op'] here you should have separated submit functions per button, because this will break if the buttons are translated to another language for example.
5. linkedin_company_access_token(): this is vulnerable to XSS exploits. Response data from Linkedin must be considered as untrusted third party provided input, therefore you must not simply concatenate the $response->error variable into the watchdog() message. Use placeholders as you would do with t(). Make sure to read https://www.drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
6. "t('There was an error authenticating with the LinkedIn network. Linkedin responded with the following error: @error | @error_description', $error)": this will not work, the second parameter to t() must be an array with the replacement variable names as keys. Same in linkedin_company_shares_get_data().
7. "watchdog('linkedin_company', $curl_error_msg, WATCHDOG_ERROR);": that will be a very cryptic error message in the logs, you should include at what situation the error happened.
8. linkedin_company_shares_requirements(): the cURL check can be removed here, you are already doing this in the main module which is a dependency for this one.
9. linkedin_company_shares_get_data(): you should first do the cache_get() and if that misses do the HTTP request. Currently the cache is pointless since you always do the HTTP request to Linkedin?
10. company-shares.tpl.php: the check_plain()s should be done in linkedin_company_shares_preprocess_company_shares(), not directly in the template.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None.

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

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

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. In linkedin_company_http_request( function you are using PHP cURL to send http get request, I would recommend you to use drupal_http_request() to hit the external URL.
2. Instead of using json_decode, better to use drupal_json_decode.
3. Also following code contains some extra space.
   

   function linkedin_company_shares_form_linkedin_admin_alter(&$form, &$form_state, $form_id) {
     $form['linkedin_company']['linkedin_company_shares']                                             = array(
   

4. Inline comments and coding approach looks good to me.

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.

As there is no blocker from my side, moving to RTBC.

@Michael Hodge Jr, thanks to you for your hard work and patience!

Hi Michael - nothing else to do at this point except wait. There is a very large backlog of RTBC modules, yours is pretty much at the back of the queue right now (you went through the review process considerably quicker than most others after you made that LinkedIn post asking Drupal LinkedIn developers to review your module - most developers patiently wait months for the review process to complete).

As you have the review bonus I suspect the Git admins will take preference over reviewing your module.

Best of luck!

Marking as RTBC again.

Automated Review

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

• 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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation; addressed.

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: 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

This is more a note to myself and other people reading this in the future, but linkedin_company_mail uses !site as a placeholder (ie, the passthrough version). This initially gave me heartburn, but this variable is is used this way in email subjects all through core.

In linkedin_company_cron(), the variable_get() should have a default value.

In linkedin_company_mail(), see https://api.drupal.org/api/drupal/includes%21common.inc/function/l/7 for a better way to hanled links in translated strings.

Once you know your file is README.md, you remove 64--69 in linkedin_company_help().

drupal_goto() can take the same options that you pass to l(), url(), etc. No need to fully build this up first.

In linkedin_company_access_token, you build the return URL for the call to Linked in. Don't build up this up with string functions. Use the query options like you have in other places.

In linkedin_company_shares_block_view() and linkedin_company_shares_get_data(), don't build up #markup. Build up a proper render array.

In linkedin_company_shares_get_data(), don't build up the full URL with string functions. Use the query options like you have in other places.

It would be best to fully build the attachment-title link in the preprocess, rather than the template.

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.

Not seeing any blocking issues, and issued mentioned my other admins have been addressed.

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.

Thanks for your contribution, Michael Hodge Jr !

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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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.