Drupal.org
Issues

Commerce Everyday

Posted by mitrpaka on December 14, 2012 at 11:15am
Project:Drupal.org Project applications
Component:module
Category:task
Priority:normal
Assigned:Unassigned
Status:closed (fixed)
Issue tags:PAReview: Commerce

Issue Summary

Commerce Everyday integrates Everyday web payment method with Drupal Commerce. Everyday is a Finnish billing and part payment online gateway provider.

Link to project page:
http://drupal.org/sandbox/mitrpaka/1866604

Link to git repository:

git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/mitrpaka/1866604.git commerce_everyday
cd commerce_everyday

Drupal 7

Reviews:
http://drupal.org/node/1696900#comment-6845714
http://drupal.org/node/1856652#comment-6846034
http://drupal.org/node/1865074#comment-6846064
-----------------------------------------------------------
http://drupal.org/node/1867860#comment-6860358
http://drupal.org/node/1867612#comment-6861716
http://drupal.org/node/1870506#comment-6861784

Login or register to post comments

Comments

#1

Posted by vineet.osscube on December 14, 2012 at 11:58am

Hi,
Firstly there is lots of issue regarding line spaces, indentation & comments.

Look at this !
http://ventral.org/pareview/httpgitdrupalorgsandboxmitrpaka1866604git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

Manual Review:

1) There is no hook_uninstall to remove your custom variables from database like: commerce_everyday_method_title, commerce_everyday_method_title_icons, commerce_everyday_method_offsite_autoredirect, etc. in admin.inc

2) use variable_del() in hook_uninstall() that remove variables or delete sql query.

#2

Posted by mitrpaka on December 14, 2012 at 2:06pm

Thanks for the comments!

- commerce_everyday.install file with hook_uninstall() added
- line spaces, indentation and comments issues addressed

#3

Posted by mitrpaka on December 14, 2012 at 6:40pm

PAReview: Commerce tag added

#4

Posted by klausi on December 14, 2012 at 7:39pm

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

#5

Posted by mitrpaka on December 14, 2012 at 8:00pm
Issue tags:+PAReview: review bonus

Review links provided and PAReview: review bonus tag added

#6

Posted by klausi on December 15, 2012 at 11:19am
Status:needs review» needs work
Issue tags:-PAReview: review bonus

manual review:

  1. "watchdog('commerce_everyday', 'Received callback for redirect validation with $_REQUEST: !request', array('!request' => check_plain(print_r($_REQUEST, 1))), WATCHDOG_INFO);": use "@" placeholders instead of "!" then you don't need check_plain().
  2. "$hash_elements[$element] = check_plain($_GET[$element]);": why do you run check_plain() on the varaibles if you are not printing them to the user? Please read http://drupal.org/node/28984 again. Same in "$hash != check_plain($_GET['OPR_RETURN_MAC'])".
  3. commerce_everyday_redirect_form_validate(): before saving the order you should make sure that the incoming request is actually valid, no?
  4. "$order = commerce_order_load($order->order_id)": why do you have to re-load the order? Please add a comment.
  5. "date_default_timezone_set('Europe/Helsinki');": why is that location hard coded? Please add a comment.
  6. _commerce_everyday_urlencode(): why is that necessary? Aren't those characters illegal in the path of a URL? Please add a comment.

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

#7

Posted by mitrpaka on December 19, 2012 at 7:36am
Status:needs work» needs review

@klausi

Thank you for the review comments. Please see some of the comments below:

1. Changed. '%' placeholder used instead of "!" and check_plain() removed.
2. check_plain() used because of security issues (to prevent potential XSS). I have now removed check_plain() after reading http://drupal.org/node/28984 again and looking drupal_get_query_parameters() API.
3. No. Transaction validation is done after saving $order with $_REQUEST data. Incoming request is saved to $order->data['commerce_everyday']['response'] in order to have data available if needed later on (e.g. for debugging purposes)
4. Idea was to have fresh copy of $order before saving incoming request data. Maybe overkill and thus removed.
5. Unintentionally left for the code after testing. Now removed.
6. You are correct. _commerce_everyday_urlencode() now removed as not needed.

#8

Posted by mitrpaka on December 19, 2012 at 7:40pm
Issue tags:+PAReview: review bonus

PAReview: review bonus tag added once again (after 3 more reviews done)

#9

Posted by tikto on December 19, 2012 at 7:40pm
Issue tags:-PAReview: review bonus

PAReview: Commerce tag added

#10

Posted by mitrpaka on December 19, 2012 at 7:45pm
Issue tags:+PAReview: review bonus

Now having both PAReview: Commerce and PAReview: review bonus tags

#11

Posted by klausi on December 20, 2012 at 9:13pm
Status:needs review» reviewed & tested by the community
Issue tags:-PAReview: review bonus

Did a manual review, looks RTBC to me now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

#12

Posted by klausi on December 30, 2012 at 9:38pm
Status:reviewed & tested by the community» fixed

no objections for more than a week, so ...

Thanks for your contribution, mitrpaka!

I updated your account to let you 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 get 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.

#13

Posted by mitrpaka on January 2, 2013 at 10:00am

Thank you klausi and osscube!

#14

Posted by System Message on January 16, 2013 at 10:00am
Status:fixed» closed (fixed)

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

nobody click here