UberCart Rabobank OmniKassa

.Install file

• Remove $id$ from first line

.Module file

• you are not using this permission anywhere - "administer omnikassa"
• In hook_order - Blank cases of load and delete, also no code in condition in save case. Please remove if they were for testing.

please use Coder module and also use ventral.org to review your module.

AJAX cart for Ubercart
http://arpiprogrammer.ru

Hi,

An automated review of the module still throws up quite a lot of issues, see: http://ventral.org/pareview/httpgitdrupalorgsandboxweb-beest1569438git

From a manual review:

ReadMe.txt should be renamed to README.txt.

In your hook_uninstall() you should remove any variables which are set by the module.

Many of your functions are missing function header comments.

I believe that you shouldn't wrap your strings with t() when in hook_schema()?

Did you mean to set that from 'needs review' -> 'fixed' ?

That isn't the usual workflow here: after you've made your fixes you need to leave the application in 'needs review' state until people have reviewed it and deem it to be 'RTBC'. At which point some of the 'Code Review Administrators' will take a look and either set it to 'fixed' and give you the relevant access permissions or put it back to 'needs work'.

See more info on the workflow here: http://drupal.org/node/532400

So I guess this needs to go back to 'needs review'? or am I missing something?

Probably he's giving up due to the long waiting times :(

Correcting status, feel free to reopen whenever you want to continue.

I'm not sure what you mean by "all open issues were fixed" - there are still very many coding standards problems, as mentioned in #1 and #4. See http://ventral.org/pareview/httpgitdrupalorgsandboxweb-beest1569438

Now any coding standard problems seem's to be resolved! No relevant errors shown at http://ventral.org/pareview/httpgitdrupalorgsandboxweb-beest1569438 (I've repeated the review)

You are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch.

You should remove the "nbproject" directory from both your remaining branches, and remove the ".gitignore" from your 7.x-1.x branch.

You can't set your own application to RBTC - that has to be done by someone who's reviewed the project.

README.txt is missing, see the guidelines for in-project documentation.

What happens with this project? It is inconceivable that he has been publishing changes from August, and has showed improvements and interest in collaborate and give something back to the community and the project is stopped :-(. Change to needs review.

Sorry, the project is ready and tested, should leave from my pov the sand box.

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 :-)

it is a very good practice web-beest, for yoursef and for the rest of the community, and in fact i've seen projects being accepted in a question of hours after simply adding a group of reviews (3 at least recommended).

Good luck :-)

@urwen: Did you really test this module? Part of the reason why highly specialized modules like this don't get approved quickly is that there are not many people who can actually test them. This module implements a payment method for a Dutch credit card payment processor. In order to even minimally test the module you would have to have an account with that payment processor. @urwen, from you location I suspect you don't. I don't either, and can't get one because of my own location. Usually in a situation like this the applicant should be the driving force to find someone who can test it, then convince that person to provide a review. Local users groups or IRC channels are a good place to look for someone for this task.

As far as I can tell from the above comments, only some superficial coding standards / automated reviews have been done, no one has actually tested the code and looked for security problems. The latter is extremely important for payment modules! @urwen, it appears you marked this issue RBTC simply due to the time it's been in the queue. If you have indeed tested it and done a deeper review, then please explain in detail what you've done.

A visual inspection of the code looks pretty good. There are still some problems, however. Specifically, the 7.x-1.x branch doesn't remove the module's system variables in hook_uninstall(), and the system variables are named differently in 7.x-1.x than in 6.x-1.x. The 7.x-1.x variables are named with the uc_omnikassa_ prefix, which is CORRECT, while the 6.x-1.x variables just start with omnikassa_. I suggest that the 7.x-1.x naming be used in the 6.x-1.x branch, but if not then there should be a hook_update_N() function in 7.x-1.x which renames all the variables, to cover the case where a user installs 6.x-1.x then later upgrades to 7.x-1.x.

#4 pointed out the hook_uninstall() problem in the 6.x-1.x branch, which has since been corrected. But the fact that it hasn't been fixed in 7.x-1.x, along with the difference in variable names, suggests that the development in the two branches hasn't been coordinated well between the two authors (one who worked on the 6.x-1.x branch, one who worked on the 7.x-1.x branch).

I personally would be uncomfortable using this on a live web site to collect payments without at least ONE person running actual functionality tests and examining it for security issues.

I have a webshop created in Drupal and a contract for omnikassa (a dutch bank payment system), but I see that the plugin is still in the sandbox, I really want omnikassa on my website, how long will test take ?, and I would like to get in touch with somebody who already works with omnikassa on his drupal site
Thank you

I work with Omnikassa on my Drupal site.
Not this sandbox module though, but using Payment module + Xano's Omnikassa module.
So if you need to have a solid Omnikassa solution implemented ASAP (like I did) I suggest you contact him.

Hi TR,

you are absolutely right blaming me. I am new reviewing modules. I am doing it mainly to learn from other's coding, but as you have seen, i don't have too much experience doing reviews.

So sorry for that, I will learn from my own mistakes. Thank you for your help TR.

Best regards.

Needs work for the issues identified in comment #26.

- inconsistent variable naming between branches
- 7.x-1.x not removing variables on uninstall.

A few extra comments:

- db_query("DELETE FROM {uc_order_statuses} WHERE order_status_id = '%s'", 'failure');
I don't know Ubercart, but this feels like it would have a large risk of conflicting with other ubercart payment methods modules who define a 'failure' status. If uninstalling this module accidently removes all 'failed' orders for a different payment method, you could have some angry site maintainers on your hands! One suggestion would be to make this order status more specific ... 'omnikassa_failure' instead of just 'failure'.

- define('UC_ORDERSTATUS_CANCELED', 'canceled');
If you're going to define constants, they should belong to your namespace instead of uc_order.module. Otherwise, someone with a uc_paypal payment method could also attempt to define a uc_orderstatus_canceled constant, and the two could conflict. Use the 7.x-1.x naming in both branches.

- I suspect that many of the images included in the 7.x-1.x branch are subject to copywrite ... I don't know whether this is generally acceptable use for those logos within the industry?

This really needs someone familiar with Ubercart to do a final approval on it, as I can't speak to whether the implementation is complete/secure from a UC perspective.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'm a robot and this is an automated message from Project Applications Scraper.

Added text to let people know about the 7.x branch.