[D7] Activity Stream Instagram

Multiple Applications

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

Project 1: https://drupal.org/node/2275135

Project 2: https://drupal.org/node/2275123

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.

Hello Denison,
You're running into the Multiple Applications issue, I am reviewing this one which is currently open for reviewing.

About the description of the project, you could put for convenience:

• a link to the Activitystream module
• the link to the git source of your project in form of:
  git clone --branch 7.x-1.x http://git.drupal.org/sandbox/denison/2272825.git activitystream_instagram
• and the pareview link:
  http://pareview.sh/pareview/httpgitdrupalorgsandboxdenison2272825git
  Even if not necessary now since you already addressed all warnings, could be useful to have it ready in case of further edits.

In the README.txt
replace all "provaided" with "provided", "regeister" with "register".

Drupal best practices have been followed, code is clear and well commented, except that seems like there is no particular reason to exploit Curl for url request instead of drupal_http_request.

Tx for your contribution!

Code is fine and working for me. Two instance of "Provaided" still there in readme.txt.

Hi! A small remark:

activitystream_instagram.module:102

  $created_date = new DateTime(date('Y-m-d H:i:s', $raw_field->created_time));

Such date object construction is confusing. date() is not needed there. Use new DateTime('@' . $raw_field->created_time) instead.

format_date is preferred in Drupal
https://api.drupal.org/api/drupal/includes%21common.inc/function/format_...

Manual review

Except #10 everything seems fine to me. Maybe you should implement hook_help() as well, but this is just a little notice.
Also a little UX idea, that you should provide a link to user/[uid]/edit/activity-stream on user/1/activity-stream page, if there isn't any activity stream have been added yet.

Nice job.

(Needs work, because of #10)

Using DateTime instead of format_date isn't a blocking issue (but, yes, it should be fixed at some point). Is there any showstoppers with this to prevent RTBC?

@denison, thankyou for your contribution.

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 Activity Stream?

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. (*) Following code looking for instagram.png but this is not in module directory. Need to fix it.
   'icon' => drupal_get_path('module', 'activitystream_instagram') . '/instagram.png',
2. (+) hook_help() is missing in this module.
3. (+) Instead of using json_decode(), better to use drupal_json_decode().
4. (+) Use format_date() in following code.
   $created_date = new DateTime(date('Y-m-d H:i:s', $raw_field->created_time));
5. In following submit function you are passing form submitted values in activitystream_account_save() without checking whether these values are safe. I think it should be checked there because activitystream_account_save() directly updating these values in database after serialize.
   

           function activitystream_instagram_form_activitystream_accounts_form_submit($form, &$form_state) {
   
           $data = array(
           'hashtag' => str_replace('#', '', $form_state['values']['activitystream_instagram_hashtag']),
           'count' => $form_state['values']['activitystream_instagram_count'],
           'client_id' => $form_state['values']['activitystream_instagram_client_id'],
           'client_secret' => $form_state['values']['activitystream_instagram_client_secret'],
           );
   
           activitystream_account_save('instagram', $data, $form['#user']->uid);
           }
           

   // activitystream.module function.

           function activitystream_account_save($service = NULL, $data = NULL, $uid = NULL) {
           $uid = isset($uid) ? $uid : $GLOBALS['user']->uid;
           db_merge('activitystream_account')
           ->key(array('uid' => $uid, 'service' => $service))
           ->fields(array('data' => serialize($data)))
           ->execute();
           }
           

6. (+) In _activitystream_instagram_pull_items function, you are using $instagram_data to hold response, I think it should be checked for secure code before pass further.

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

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.