[D7] LinkedIn Group Posts

welcome

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See http://ventral.org/pareview/httpgitdrupalorgsandboxchhavik1379612git.

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. Go and review some other project applications, so we can get back to yours sooner.

If you got any questions on this please ask!

I found stuff like drupal_get_path('module', 'linkedin_group_posts') in your .tpl file. Rather than using such functions in a tpl you should create a preprocess function to generate these values ready-to-use. This makes theming your module more flexible and much easier to understand for themers with limited PHP knowledge.

Your checking the existance of the oauth library in hook_init() meaning that it will be done on every page request, maybe you rather put that on the status report page (See hook_requirements)
Also you add your css file here, is it really necessary to add it on any page? I don't think so, you should rather add it when it's really needed.

Your using hook_form_alter() but you could also use hook_form_FORM_ID_alter(), looks better ;)

$oauth_token = variable_get('linkedin_oauth_token_key', '');
  $oauth_secret = variable_get('linkedin_oauth_secret_key', '');
  $group_id = variable_get('linkedin_group_id', '');
  $count = variable_get('linkedin_posts_count', '5');
  $url = "http://api.linkedin.com/v1/groups/$group_id/posts:(id,title,summary,creator,creation-timestamp,site-group-post-url)?count=$count&start=0&category=discussion&order=$order_type";
  $tokens = array('token_key' => $oauth_token, 'token_secret' => $oauth_secret , 'type' => 'access');
  $signature = new OAuthSignatureMethod_HMAC_SHA1();
  $consumer_key = variable_get('linkedin_consumer_key', '');
  $consumer_secret = variable_get('linkedin_consumer_secret', '');
  $consumer = new OAuthConsumer($consumer_key, $consumer_secret, NULL);
  $token = new OAuthConsumer($tokens['token_key'], $tokens['token_secret'], 1);
  $request = OAuthRequest::from_consumer_and_token($consumer, $token, "GET", $url);
  $request->sign_request($signature, $consumer, $token);
  $header = $request->to_header("https://api.linkedin.com");
  $response = _linkedin_group_posts_http_request($url, $header);

Woooohaa, could you add some comments here ? ^^ (generally more comments wont hurt your module ;) )

regards

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

CCK Combo: http://drupal.org/node/1161214
LinkedIn Group Posts: http://drupal.org/node/1379794

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, I have marked your secondary applications as 'closed(duplicate)', and left one application open (chosen at random).

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

Thanks in advance for your patience and understanding!

In http://drupal.org/node/1127732 it says when cleaning up the master branch to do:

echo See major version branches.> README.txt

Also when I checkout your code I'm only seeing the master and when I type git branch I don't see the other branches. This could be my error? I will check with some folks.

pareview didn't show any errors which is great. I've changed it to needs work just so you know to change the readme as I've noted above. I'll load up the module to take a look.

I installed it locally, was ready to go and then saw for the linkedin api you need an external url for your site. So I set it up again on a url I have available, and as soon as I point to the Oauth.php file the site gives me a white screen and says this when I run drush status:

Error: Cannot redeclare class OAuthException in
/var/www/cfannon_babson_trunk/docroot/sites/all/modules/contrib/linkedin_group_posts/OAuth.php, line 8

I know this is due to an error with the linkedin module and not yours, but I cannot do further testing without this resolved, I get a white screen of death. Do you have a solution for this? If you do I will continue to do the review. If I have a bunch more time, I will work on the problem with the fact that the LinkedIn module doesn't namespace all functions it uses, submit a bug to the author, etc., or whatever I find appropriate. I'm sure the OAuth.php functions should be declared as a library to prevent these conflicts, since multiple modules use OAuth.

I would suggest you make it so your module does not suffer from such namespacing problems by working with the author of the linkedin module or crafting your module to not depend on this being resolved to work.

If I'm missing something and you have the fix for this let me know thanks!

Here is the link to the libraries API: http://drupal.org/project/libraries

Yes I know it probably works for you, but because the OAuth.php file doesn't use namespacing, and other contributions on drupal.org do not obey the rules in namespacing their modules I cannot install your module without sorting through that big problem. The OAuth.php library should probably be using the library api.

Just a guess, but given that the LinkedIn Integration project reports 451 sites using it, I suspect that the conflict most likely has something to do with the particular review environment, and not the LinkedIn Integration module itself.

I took a look at the test site you setup. It looks very nice. At some point I will try to find time to identify why the linkedin module will not load for me, I'm sure there are many others in the same boat, and this will be helpful.

So I ran an automated review:

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

FILE: ...s/all/modules/pareview_temp/test_candidate/linkedin-group-posts.tpl.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 9 | ERROR | File doc comments must be followed by a blank line.
--------------------------------------------------------------------------------


FILE: ...sites/all/modules/pareview_temp/test_candidate/linkedin_group_posts.css
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 26 | ERROR | Expected 1 space after colon in style definition; 0 found
--------------------------------------------------------------------------------


FILE: ...es/all/modules/pareview_temp/test_candidate/linkedin_group_posts.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 58 | ERROR | Array indentation error, expected 4 spaces but found 6
 61 | ERROR | Array indentation error, expected 4 spaces but found 6
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

I'm on vacation this week and will do a few reviews occasionally this week.

Doing a bit of manual review I see:

84 /**
85 * Callback function to generate block content.
86 *
87 * @param type $order_type
88 * Whether the post type is 'Latest' or 'Popular'
89 */

there is no return value doxygen documentation in this block? That doesn't look right to me.

Those helper functions in linkedin_group_posts.module also might be called out by the admins here for lack of documentation? And using curl instead of drupal_http_request() got me a comment in my module: http://drupal.org/node/1328366#comment-5490924

The demo site looks good!

@barnettech can you list down the steps that you followed to install the linkedin module? I followed the README of linkedin module and was able to get both the modules running on my local system.
I don't think using cURL is a problem here. I think having a cURL test in the hook_requirements should be enough.

@chhavik I can see the blocks and the module is working.

But there are a few usability issues:

* The README.txt does not clearly list the steps to configure this module.
* You don't mention how one can obtain the OAuth token and OAuth secret. We can do that by using the Linkedin profile integration module.
* Why does the user need to open the linkedin_token table and fetch the OAuth token and secret values? This is a serious usability issue.
* I suggest you have these values pre-populated in the form and let the users enter only the group id and the number of posts settings.

@barnettech were you able to get the linkedin integration module running? I see 474 sites using it. Guessing that it could be an issue with your testing environment.

@chhavik the module is much easier to use now.
PAReview gives no errors. Went through the code and found nothing of note . Marking it RTBC.

Review of the 7.x-1.x branch:

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

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. Get a review bonus and we will come back to your application sooner.

manual review:

• "$block['subject'] = 'Latest Discussions';": all user facing text should run through t() for translation. Also elsewhere.
• OAuthSignatureMethod_HMAC_SHA1: where do the oauth classes come from? Do you depend on oauth? In your info file you only depend on linkedin.
• _linkedin_group_posts_http_request(): if you depend on cURL should implement hook_requirements() like here http://api.drupal.org/api/drupal/modules!simpletest!simpletest.install/f...
• _linkedin_group_posts_parse_fields(): same here, don't check for xml_parser_create, this should be done in hook_requirements().

Hi,
Sorry for the delay,
as there are currently many applications in queue we need more reviewers,
so think about getting a review bonus and we will come back to your application sooner.

Review of the 7.x-1.x branch:

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

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. Get a review bonus and we will come back to your application sooner.

manual review:

• do not use extrat() without the second parameter! The data you are receiving is untrusted input, so you should be careful what variables could be overridden by the extraction.
• linkedin-group-posts.tpl.php: do not create image markup yourself, use theme('image', ...) instead.
• linkedin-group-posts.tpl.php: The values you are printing are not sanitized anywhere, as they are coming form an untrusted third party you need to do that. See http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-... Or am I missing something?

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

Please add your review links to the issue summary. Please set the status accordingly if you have reviewed an application ("needs work" or "reviewed & tested by the community").

• Your extract() call is wrong. You should NOT overwrite existing variables and currently your third parameter does not make sense as it is used in conjunction with the second parameter. This is a security issue as you could overwrite any variables if you get a malicous response from the third party service. Please read the documentation of extract() again.
• linkedin_group_posts_get_group_data(): you should use the @return syntax to describe the returned data of that function.

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

Reviewed the code base and found the following issues

1. linkedin_group_posts.module line no 156: The to_header function throws OAuthException which is not properly handled. So if this method throws the exception your request will have a wight screen of death :)
2. linkedin_group_posts.module line line 207: Again you haven't checked any curl response codes before you sending over the output. Should use curl_error or the response headers using curl_getinfo(, CURLINFO_HTTP_CODE) to check for the response first.
3. I cant see the whole point of converting a XML doc to an array in _linkedin_group_posts_parse_fields. Cant you just using simplexml_load_string and access the elements you require ?. You can find some more documentation on http://php.net/manual/en/function.simplexml-load-string.php
4. Incorrect usage of watchdog function in line 171 in linkedin_group_posts.module. Please see the general practice for the $type parameter in http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/watchdog/7
5. Incorrect usage of the Drupal Form API. There is no #value defined for the textfileds. #value is only used for non user editable fileds. Please look at http://api.drupal.org/api/drupal/developer!topics!forms_api_reference.ht...
6. Variables defined in line 233, 234, 235 $parents, $opened_tags, $arr has never been used.
7. Is the dependency for linkedin module is solely to use the OAuth.php library ?
8. Also I have noticed that lack of sanitation throughout the module. It is always a best practice to validate your data before you do anything with it. This will prevent the application from throwing out arbitrary warnings and notices and also make your life easier when you maintain it :).

manual review:

1. nice, now this is much shorter and easier to review :-)
2. "drupal_set_message(t("Curl error:") . curl_error($ch), 'error');": do not concatanate translatable strings, use placeholders instead. Also, what does curl_error() return and is it trustworthy? If not then you might me vulnerable to XSS exploits, if curl_error() passes through any third party provided data that might contain malicious script markup.
3. "drupal_set_message($e->getMessage(), 'error');": Same here, are you sure that you don't need to sanitize the error message before printing it?
4. Also, are you sure that you want to print the error messages to all the end users that might see the block? This could reveal sensitive information about your site setup and it is strongly recommended to not show such error messages on production sites. Maybe you should check for the debug mode everytime before you use drupal_set_message()? Or better use watchdog() instead in _linkedin_group_posts_http_request().

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

Awesome, thanks for all your hard work! Looks RTBC to me now. Assigning to tim.plunkett as he might have time to finally approve this.

Let's finish with this.

Your project page is pretty short, you may have a look at the tips for a great project page. You may merge your README and project page, so both contain general and installation instructions.

Please take more care about your function documentation, "Some internal helper functions..." is not very helpful.

Read the watchdog documentation about the $message string (it must be untranslated) and passed variables. The message severity comes after the variables array!

$curl_error_msg = t("No HTTP code was returned");
watchdog('linkedin_group_posts', $curl_error_msg, WATCHDOG_ERROR);

should bewatchdog('linkedin_group_posts', "No HTTP code was returned", array(), WATCHDOG_ERROR);

and

$message = t('Linkedin debug : LinkedIn.com answered "@status : @message', $response_message);
if (variable_get('linkedin_debug_mode', 0) == 1) {
  drupal_set_message($message, 'warning');
}
watchdog('linkedin_group_posts', $message, WATCHDOG_ERROR);

should be:

if (variable_get('linkedin_debug_mode', 0) == 1) {
  drupal_set_message(t('Linkedin debug : LinkedIn.com answered "@status : @message', $response_message), 'warning');
}
watchdog('linkedin_group_posts', 'Linkedin debug : LinkedIn.com answered "@status : @message', $response_message, WATCHDOG_ERROR);

Note that l() is using same options as url():
print l(t('View all'), "http://www.linkedin.com/groups?gid=$group_id", array('attributes' => array('target' => '_blank')));should be:print l(t('View all'), "http://www.linkedin.com/groups", array('query' => array('gid' => $group_id), 'attributes' => array('target' => '_blank')));

After all I'm sure you'll fix these asap, but please take more care about the API documentation.

Therefore, thanks for your contribution and 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.