[D7] Commerce EST Direct

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.

Multiple Applications

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

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

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

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.

You need to update your Git link. What you've provided is for your use. The general link should be:

http://git.drupal.org/sandbox/magicleaves/2271885.git

Code Standards

You having a minor coding standard error:
(http://pareview.sh/pareview/httpgitdrupalorgsandboxmagicleaves2271885git)

FILE: /var/www/drupal-7-pareview/pareview_temp/commerce_est_direct.module
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
4 | ERROR | Doc comment short description must end with a full stop
--------------------------------------------------------------------------------

Manual code review

It appears that there are messages in commerce_est_direct_transaction that should be enclosed in t().

Given that the code in commerce_est_direct.module looks like a typical form callback, but in fact appears to be a callback for the commerce module, I'd prefer more details in the function definition docblock explaining what they do and how they link with the commerce module.

Fixed git link.

Automated Review

PAReview complained about:

FILE: /var/www/drupal-7-pareview/pareview_temp/commerce_est_direct.module
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
155 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------

The problem line:

drupal_set_message(t(check_plain($response_detail->getErrorMsg())), 'error');

This won't work, the t() function cannot handle a function call.
The call to check_plain should be enough.

Manual Review

Individual user account

No. It needs clarification whether this application follows the guidelines for individual user accounts.
The account is tied up to a company webpage http://magicleaves.com/ that uses the pronoun "We". However, it may well be that this is a one person company and that "We" is just marketing. But applicants need to clarify this.

No multiple applications

Yes

No duplication

Yes

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

No. The library it requires is supposed to contain three files (two PHP-files and one "version.txt"), but unpacking the zip-file also unpacks something that appears to be junk for "__MACOSX" and a binary file with unknown content named ".DS_Store". I don't like having undocumented files on my site. The library is download from the OPs website, so it is also unclear whther this is third party code. If the library isn't third party code, it should IMHO be bundled with the project. See also the guidelines for 3rd party code.

README.txt

No. Navigation instructions for finding the admin interface (point 4 under INSTALLATION) is given as an URL, assuming non-clean URL. Also, navigating to the path admin/commerce/config/payment-methods after installing and enabling the module and the library did not work on my site. The information in README.txt seems inadequate for someone unfamiliar with the Commerce project. It should be said that it follows the formal guidelines for in-project documentation and the README.txt Template, but the content is not good enough.

Code long/complex enough for review

Maybe. The code stripped of comments and whitespace is 130 lines long and most of it boilerplate. However, that I think it follows the minimum guidelines for project length and complexity, but a more senior reviewer may need to make the final call on this.

Secure code

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

Coding style & Drupal API usage

Preliminary findings (need to get it working before doing a full review):

• There is no hook_help(). It is good coding practice to have this hook for every enabled module.

Automated Review

The automated review of your project by PAReview has found some issues with your code. As coding standards make sure projects are coded in a consistent style we ask you to please have a look at the report and try to fix them.

Manual Review

Individual user account

Yes. Follows the guidelines for individual user accounts.

3rd party code

Yes. Follows the guidelines for 3rd party code.

README.txt

No.
One of steps under CONFIGURATION says:

 * Under the Payment Methods Administration page 
   (http://www.yoursite.com/?q=admin/commerce/config/payment-methods) 
   edit "MagicLeaves EST Direct Payment" rule.

This is an URL. the path to the admin interface is never given using an URL. You can use the path or breadcrumb (or both), but never an URL.

Code long/complex enough for review

See #10.

Secure code

Postponed (need to get it working before doing a full review).

Coding style & Drupal API usage

Preliminary findings (need to get it working before doing a full review):

• (*) The list of dependencies fails to mention the following: entity, rules, addressfield. See: https://www.drupal.org/node/542202#dependencies
• (*) Implementation of hook_help() does not work (refer to Drupal API to see how to do this right).
• (*) Navigating to the path admin/commerce/config/payment-methods to configure module do not work (I am just shown the default admin page). This is clearly as bug. I am unable to configure this module, and therefore unable to complete the review.

The starred items (*) are fairly big issues and warrant going back to Needs Work.

I am removing the PAReview: Bonus tag. As far as I have been able to tell, you have not done any manual reviews, you've just copied the output of the automatic PAReview. Please do three manual reviews before putting the tag back.

Hello,

Thanks for this module. I have not reviewed it yet. Because I saw this https://www.drupal.org/node/1914020 commerce EST.

Duplication ? Modules with the same goal/approach ? Different architecture ?

Postponed according to comment #16.

Removed automated review links form the issue summary, you have not done any 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.

Automated Review

PAReview came up almost clean (it complains about a not helpful commit message, but this is not a blocker).

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

There is another Commerce EST project (https://www.drupal.org/node/1914020) in the application queue, but it is currently blocked by a third party code issue. While the two modules may do the same functionality but the backend and the future plan differs. IMHO, the existence of the other application should not block this from becoming a full project.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template.

README.txt does not match actual behaviour of module. Referring to the following in README.txt:

   From menu go to
   Store settings->Advanced store settings->Payment methods and 
   edit "MagicLeaves EST Direct Payment" rule. In here edit the 
   "Enable payment method: MagicLeaves EST Direct Payment" action with the 
   information provided from your virtual pos supplier. Click Save. 

However, "Advanced store settings" does not appear under "Store" (only "Configuration" and "Currency settings"). See attached image for screen shots.

Code long/complex enough for review

Maybe: Follows the guidelines for project length and complexity.

The code stripped of comments and whitespace is 130 lines long and most of it boilerplate. However, I think it is long enough to make the grade regarding minimum guidelines for project length and complexity, but a more senior reviewer may need to make the final call on this.

Secure code

Postponed until README.txt is sorted out.

Coding style & Drupal API usage

Postponed until README.txt is sorted out.

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.

Changing status.