Description

This module allows site users to make donations using their Interswitch bank cards. The design is of this module has been successfully checked for all the mandatory requirements of Interswitch's User Acceptance Test as described in Interswitch's DIY document
Features:

  • Specify donation areas
  • Configure the module with your MAC key, Product ID and Pay Item ID
  • Specify whether to use Interswitch test servers or live servers
  • Get notifications when a successful or unsuccessful donation attempt is made (the same goes for the user)
  • View donation history of all users
  • Filter donation history by date and/or status
  • Download donation history in CSV
  • Users can view their own donation history
  • Manually update a donation's transaction information using Interswitch's web service

Project Page

https://drupal.org/sandbox/Feyisayo/2086825

Git Repository

git clone --branch 6.x-1.x http://git.drupal.org/sandbox/Feyisayo/2086825.git

Comments

alexmoreno’s picture

Hi feyisaso, and thank you for your effort,

firstly, your git clone instructions are wrong, use the public ones please.

You have a lot of issues, mainly coding standards, please review them so it will be easier to read and review your code:
http://pareview.sh/pareview/httpgitdrupalorgsandboxfeyisayo2086825git

Your git main branch in the -dev one. Change it to use the 1.x instead.

Lastly, help with at least 3 reviews (more recommended):
https://drupal.org/node/1975228

once done, add yourself the bonus review tag and your module will be reviewed a lot quicker.

Thanks again.

alexmoreno’s picture

some more advices regarding your code.

Your .module is quite big, and in the opposite the inc is very small. All your hook functions should be placed in the .inc.

On the other hand, to help the readibility, I'd place the .js in a /js folder, instead of using the includes.

In lines 824 for example, you have a lot of html mixed in the php code. You can use different techniques to move this html to an html template (mixing html and php is ugly, and should be carefully used).

In line 736 you have some potential injection security holes. Add this variables in the db_query function and change:

WHERE user_id = '{$user->uid}'";

with something like:

WHERE user_id = '%s'";

In line 692 you use a default timezone. Is there any way to change this in the module if I am, for example, in London?

Thanks a lot for your work, do not forget about contributing with some third module reviews :-).

PA robot’s picture

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.

gaurav.pahuja’s picture

Please find below some of my initial reiew comments:

  1. Add function level comments to _requery_isw
  2. Callback 'admin/interswitch-donate/ajax/requery' should have one more parameter. This woul avoid need to check for arg(4) at callback function level.
  3. function _goto_interswitch: Try to move HTMl into a separate template file
  4. It is always good practice to start function names with module name in order to avoid clash with other modules.
gaurav.pahuja’s picture

Status: Needs review » Needs work

Changing status to needs work.

feyisayo’s picture

Thanks guys. I will implement these changes.

This is my first Drupal module so I don't know how well I can review the code of other developers if mine has not passed the review test.

But I will pitch in with reviews wherever I can.

Thanks a lot guys. I get to these changes as soon as possible

Feyisayo

feyisayo’s picture

Issue summary: View changes
alexmoreno’s picture

Hi,

It is exactly why it is your first modulethe reason why it is important to do some recurved, learn the process Ab learn about coding in drupal :-)

feyisayo’s picture

Hello guys,
I have incorporated the changes:
1) Menu hook callback functions have been moved to the inc file
2) Removed embedded HTML except for hook_mail and t() calls.
3) Refactored the code to adhere to Drupal coding standards

Please let me know your thoughts.

feyisayo’s picture

Assigned: Unassigned » feyisayo
Issue summary: View changes
feyisayo’s picture

Assigned: feyisayo » Unassigned
feyisayo’s picture

Issue summary: View changes
feyisayo’s picture

Issue summary: View changes
Issue tags: +PAreview: review bonus
feyisayo’s picture

Issue summary: View changes
feyisayo’s picture

Status: Needs work » Needs review
klausi’s picture

Issue summary: View changes
Issue tags: -PAreview: review bonus

Removing review bonus tag, you have not done all manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

feyisayo’s picture

Klausi, I did not just post the output of the automated review tool. I read their code. The reviews I gave was all I could at the time given my nascent level of expertise with Drupal. But I did read their code. I did.

I also submitted a patch to a published module at https://drupal.org/node/2204967#comment-8566091 but that appears to have been removed. I came to know of that bug because I am using that module for a client and I submitted a patch (after many unsuccessful attempts)

Please reconsider your stance Klausi.

guardiola86’s picture

Bartuc’s picture

Status: Needs review » Needs work

Hello,

I did a quick run through. Hopefully at least one of the comments below is helpful.

  • In your CSS file, do not attach units to zero-values. 0px should be 0. Alphabetize your properties if you can (though not a coding standard for Drupal).
  • The spacing on your form elements in interswitch_donate.inc are spaced too far. lines 88-136 have 4 spaces. There should only be two.
  • The tabs should only be two spaces on line 209 - 214.
  • Spacing 495 - 498
  • Spacing 555 and 556
  • Spacing 644 - 646
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.

feyisayo’s picture

Status: Closed (won't fix) » Closed (fixed)

Further development has ceased on this module.

feyisayo’s picture

Issue summary: View changes