[D7] Credit Card on Delivery for Ubercart

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxth_tushar2146475git

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.

Looks good to me.

I've not tested module by installing.

The module code looks good, except that there should some extra line breaks at some places to make the code more readable.

@th_tushar, that's completely fine.

Adding line breaks where ever required would make your code more readable if you do so, that would also help other developers who are looking at you code and trying to understand what you've doing, especially the credit_card_on_delivery_form() function and "order-submit" and "order-save" cases in credit_card_on_delivery_payment() function.

Removing review bonus tag, 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.

This unquestionably should not be a full project on drupal.org.

I've reviewed the code, and it's straight copy of the Ubercart core "Cash on Delivery" payment method. The only difference is that "cod" and "cash on delivery" have been changed to "ccod" and "credit card on delivery" respectively. The hook_update() function was even copied verbatim, even though that function was for updates from Ubercart 5.x-1.x to Ubercart 6.x-2.x and is completely unneeded for this "new" module.

As a simple copy, this module does nothing to demonstrate the applicants knowledge of the Drupal APIs or any other thing that the review process is screening for.

This module contains absolutely no functionality that isn't contained in the core Ubercart COD module. All it changes are some strings that display to the end user, and those can be easily changed either in the Ubercart UI or through string overrides without requiring a custom module, let alone a custom module that's just a copy of the Ubercart core COD module.

No. After reading what you said, I went and took another detailed look just to be sure, and I stand by what I said in #17. You've added no functionality whatsoever. None. Not a bit. You haven't added anything about surcharge or type of card available or anything else. All you've done is change the name. You haven't even changed the messages that are shown to the user. Despite all the talk about 4G card readers and posting back to the server, this module is exactly the same as the built-in Ubercart COD payment method. Which is why you should use the Ubercart COD method with "Cash on Delivery" payment method label changed to "Credit Card on Delivery" by String Overrides or by the Ubercart UI, or at last resort by a one-line theme function. This would be 100% identical to what you've done, and wouldn't involve a custom module.

This unquestionably should not be a full project on drupal.org, and git access should not be granted on the basis of this application.

Sorry, it really doesn't matter. The COD module can already be used for credit card on delivery, which is what you say is the purpose of your module. With the COD method all that matters is if the payment is received or not, and when it was received. It doesn't matter whether the customer pays with cash, check, money order, credit card, chickens, or a combination of these, as that is a transaction that takes place off the web. All the site admin can do is record that the payment is received by the driver or by the customer when he/she walks in to pick up. And COD even has a field on the admin form so if you really want to record that the customer paid with chickens, you can enter that. Or enter anything else you want about the transaction, including card type (although I don't see how that's useful). As a maintainer of Ubercart, I think I know what I'm talking about here. Your module does nothing more that what we already have.

Regardless, this project does not demonstrate that *you* have met the requirements for git access - all you've done is copy a working module from someone else.

I still believe this should not be approved. Maybe someone else will approve it, but I hope not. I'm not going to review it anymore.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 6.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/credit_card_on_delivery.module
  --------------------------------------------------------------------------------
  FOUND 6 ERROR(S) AFFECTING 6 LINE(S)
  --------------------------------------------------------------------------------
    57 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
    66 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
    82 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
   115 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
   228 | ERROR | Functions must not contain multiple empty lines in a row; found
       |       | 2 empty lines
   237 | ERROR | Missing function doc comment
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. project page is too short, see https://drupal.org/node/997024
2. the project page should mention that this module is a straight copy of the COD functionality and that people can easily use that with string overrides instead of yet another module.
3. Since you mostly copied code this module is too small to approve you as git vetted user. However, we can promote this single project manually to a full project for you.
4. "$output .= t('Card Type:') . $order->payment_details['card_type'];": do not concatenate variables into translatable strings, use placeholders with t() instead.

The project page is a blocker right now, otherwise this looks almost ready.

Cool, then this is RTBC IMHO. Assigning to ELC as he might have time to take a final look at this.

One of the purposes of the project application process is to encourage collaboration over competition by discouraging needless duplication of module functionality. This application is not just a duplication, it's an outright copy with no added functionality. Since when have we started approving things like this? Moreover, it's a demonstration that the author does not understand the basics of Drupal - instead of using standard Drupal mechanisms to customize the existing module (e.g. changing the string through the module-provided UI, overriding theme functions, or using modules like String Overrides) he simply hacks the existing module. This is the wrong way to do things in Drupal, and is a bad example for others who are trying to learn.

Why do I care? Because as a maintainer of Ubercart, I am one of the people who has to deal with support questions for all kinds of modules like this which are not part of Ubercart core - when something goes wrong, users post in the Ubercart issue queue, and it takes time and effort and several rounds of posts to determine that the user 1) is using a contrib module, and 2) the error is in the contrib module. I don't need the extra confusion caused by this module which provides nothing over what is already in core Ubercart. On the other hand, I greatly welcome applications that add needed functionality to Ubercart. In fact I reviewed one such application and marked it RTBC less than an hour before I initially marked this one as "won't fix". It would be great if excellent applications like #2151743: [D6] UC Crowdfunding could be approved promptly, instead of devoting so much attention to ones like this which give nothing to the community.

By approving this application, you would be doing a disservice to the applicant, because instead of helping him learn how to operate within the structure of Drupal and be effective at customizing Drupal using all the many existing ways built into Drupal, you're encouraging the "hack core" mentality that we try so hard to stay away from.

I have to agree with TR on this one. This module is line for line the uc_payment_cod module from UC core uc_payment_pack, except with "Cash on Delivery" replaced with "Credit Card on Delivery" and the like, especially at its origin. It is duplication of an existing module with small changes better implemented using the Drupal and Ubercart APIs.

There is a difference in that it collects the CC type the shopper wishes to use on delivery, however the storage of the value is not quite right and is not all that useful to the store or shopper. The information is collected at the same time as the delivery date, but is then stored in a separate table with the exact same primary key. All of the values are present when changes are made to both of these tables, making it a bit of tack on. The stored CC type is never again used by the site in processing, and is completely irrelevant to the final purchase process in person as a different card could easily be used at delivery and the site would never know. I could see the utility in limiting the user to a selection of cards available from the CCOD provider so there was no surprise on delivery, but not storing it.

COD is usually not just "Cash", but many payment methods depending on the COD provider. It could be argued the "Cash on Delivery" should simply be changed to "Collect on Delivery" and state what the COD provider's terms are in the help text. This can be done by using string replacements as these terms are all translated, as TR suggested.

Given the small amount and percentage of code which can be attributed to the author, and how they are implemented, this module does not provide enough positive insight to promote them. All of the functionality could have been achieved much more simply by altering the COD module using the appropriate API methods. As such, this module in its current state and direction should not be accepted as new functionality to be published on d.o, or as a gauge for promoting the author without large changes.

There are however a couple of ways forward for this module:

• Start again, but use the API to alter COD into CCOD, or even better, just Collect on Delivery which is much more all encompassing, especially given the vast differences in COD providers. Or even better, talk to UC and see if it would accept a Collect on Delivery patch.
• Submit a patch to UC Core uc_payment_pack to add the ability to it to have a dynamic number of COD, other, and Check methods in which all of their string values can be edited. This is actually functionality I have been looking at writing myself as I have had a number of situations where having 2 or more COD or other payment methods with slightly different help text and fees would have saved me a lot of time. You would need to work with TR to ensure it would be accepted.

Please don't get discouraged, but I hope you can see that copying and pasting part of another module into a new one for a project application is not a good solution in the circumstance. We want to know that you have the ability to write your own secure and unique code, and that you understand how to best use the core and contributed modules' APIs.

@ELC: I already stated that we cannot give the git vetted user role away with this application, since it is just a copy of existing code. But we can always promote this project manually.

I think the emphasis is that we prefer collaboration over competition, but we do not enforce it. So if an applicant insists on forking a module (or a payment method) we can give advice why that should not be done and we can demand documentation of that fact on the project page - but it is not our place to forbid forks.

@th_tushar: Commerce Credit card on Delivery is different because the author has also made other useful contributions to other modules, so I think that was justified.

So we have a disagreement here and according to the Drupal code of conduct we should reach out to others to move this forward.

I was pointed to the Technical Working Group, so I opened #2157047: Project approval conflict to get help on this.

I have read this issue but not reviewed the code myself. I don't think anyone is disagreeing about the contents of the code so reading/reviewing the code seems unnecessary.

I agree with Klausi's comment in #32. It's not the place of this queue to forbid forks. It also appears, based on the reviews of the code, that this module doesn't meet the goals of this queue to give th_tushar the git vetted user role.

I read through this issue and the sandbox project's code, and I agree with klausi in #32.

We aren't the module police (we just look like them sometimes :)) so I don't think it's appropriate to prevent this module from being promoted. The author has a note on the project page explaining the similarities between this and the cash on delivery module. The project age also contains a reasonable use case.

However, we clearly cannot grant "git vetted user" status based on code the applicant did not write. There do not seem to be significant changes and no other code is available to review.

So @ELC and @TR: can you live with that reasoning? Is it acceptable for you if we proceed with a manual project promotion here?

Frankly, no, I don't understand the urgent need to promote this. I have yet to hear a reason why this should be a full project rather than just living in the sandbox. There's nothing wrong with being a sandbox project.

Full projects should be different in *some* way from sandbox projects. If there is no filter which prevents some sandbox projects from becoming full projects, then what's the point of this application process? Is this just a fraternity hazing, giving everybody a chance to paddle the pledge, or are we trying to actually accomplish a goal here? If we are not allowed to say "no" at all, then the process has little meaning and simply wastes the time of the applicants and of the reviewers.

But I don't see the point of debating here what the application process is or should be. As it stands, the module doesn't meet the established criteria to become a full project.

In the 5 years or so I have been reviewing applications, I have never once until now thought an application should be outright denied. I have mainly reviewed Ubercart modules because they tend to require specialized knowledge and are therefore somewhat neglected in the application process. I have that specialized knowledge, so that's where I can be most effective in this queue. This being a community review process, I would hope that the community could respect my opinion on this particular module, just like I respect and accept the decision of others when they mark a project RTBC, or close a project for lack of activity - if we all go around undermining each other's reviews then this will become a dysfunctional chaos. At the very least, if someone is going to come along and completely reverse the decision I made after spending time and effort to review this module then I would appreciate a pretty good reason why the decision should be reversed. That didn't happen here.

I can think of dozens of cases over the years where an applicant submitted a fully-formed piece of code, intending only to donate the code to the community, but was unwilling or unable to support that code going forward. Those applications had some significant and useful code, yet they were denied full project status and the Drupal community lost those contributions. In contrast, this module has nothing useful, yet you propose to approve it anyway? Why? Just because the applicant intends to support it? (Although the fact that it's an unnecessary copy of a five-year-old D6 module when D6 is nearing its end-of-life doesn't bode well for future support - this will likely be one more outdated and abandoned module a year from now.)

I don't see anything wrong with *sharing* this code, but sharing can be done in a sandbox. Promoting it to a full project tells all other Drupal users that it's something that might be useful to the general community. That's not the case here.

@kscheirer: No, there is no reasonable use case on the project page. He give an overview of functionality which ALREADY EXISTS in core Ubercart. That's not a justification for a new module. Letting the customer pay with a credit card and specify a desired delivery date already exists, for example. The ability to record the card type actually used already exists. And if he wants to add a credit card logo to the checkout page, the Ubercart COD module provides a UI for the admin to enter any desired descriptive text that will be presented to the customer. This defaults to "Full payment is expected upon delivery or prior to pick-up.", but it's TRIVIAL to change this to include text about what forms of payment you will accept, which credit cards, whether or not personal checks are accepted, etc. The UI textarea accepts full HTML, so credit card images can be embedded here. Or if you want the customer to select it can be done with a two-line hook_form_alter() - but NOT with an entirely copied module with two extra lines added in.

This really is a fundamental point, and central to the mentoring goal of the application process - the applicant really doesn't seem to understand that there are like 10 different Drupal-standard ways to customize the existing COD module to do EXACTLY what the applicant wants, with only a few (or NO) lines of code. Given that, there's nothing left to review. The application process fails in this mentoring goal if we can't transmit that one little piece of information.

I'm not trying to be the 'module police' here. I hope you can agree with me that one of the reasons we have this application process is to improve the quality of Drupal's 'full-project' codebase, to make it more useful to the community, and in general increase the signal-to-noise ratio in the project repository. Approving this module would go the exact opposite direction. We already have a place for unmoderated code - it's called the sandbox. Encouraging people to put *anything*, however useless, into the full project repository is counterproductive.

I have no idea why you guys are fighting so hard to get this approved, I can only say that it *seems to me* that there's something else going on here, that the argument for approval is not being made based on the stated goals of the application process, nor on the precedents that we've established for this process over the years, but rather on the desire to change the process or change the nature of what is considered a full project vs. what is considered a sandbox. Again, that's what it appears to *me*, because in *my* view there's no question - none at all - that this project application contributes nothing to the community and should be left in the sandbox.

The reason this should be a full project is that the applicant wishes to do so and that there are no security or licensing issues with the code (the only two reasons that are hard application blockers in our current process).

The point of this process is to prevent name space squatting on drupal.org and prevent additional work for the security team. Side goals: educate and mentor applicants, encourage collaboration etc.

As I understand it you are trying to make a point that by approving this project we would do harm to the Drupal community, i.e. it might make your life as Ubercart maintainer a bit harder and site builders researching Ubercart extensions could get a bit more confused. So the question is if that is enough to block this application.

I respectfully disagree with your conclusion (by which I am in no way undermining your review skills, you are doing a great job!), so I think we need to invoke the Technical Working Group to make a decision for us, since we cannot.

I saw you linked an issue klausi, but the process is unfamiliar to me. Is there anything else needed there?

I don't think so. See also https://drupal.org/project/governance and https://drupal.org/governance/technical-working-group (the resolving conflicts section).

Multiple Applications

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

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

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

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.

Multiple Applications

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

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

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

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.

@th_thushar - I think this issue will still get a response from the TWG. At least, I think it deserves one. That will happen regardless of the fact that the bot is closing it.

Multiple Applications

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

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

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

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.

Hi th_tushar and project application reviewers,

This is Alex Bronstein from the TWG. First of all, sorry for the delay in responding to this. We're still a new group and figuring out our processes for making and communicating decisions. For this application, it seems there's consensus among reviewers in this issue that the submission does not show enough original code to grant th_tushar the "git vetted user" role, so this response will focus only on whether the individual project should be promoted. In evaluating this question, the TWG focused on the two points of contention identified in the issue: i) the validity of the module’s use case, and ii) the appropriateness of the actual implementation.

Regarding the use case, the module supports listing both ‘Cash on Demand’ and ‘Credit Card on Demand’ payment methods separately within the same form. Comment #37 suggests that it would be trivial to implement an alter hook to change the ‘Cash on Delivery’ label to a generic ‘Credit on Delivery’ option. The TWG sees merit in that, but also recognizes that it does not directly support the module author’s desired use case of simultaneously supporting separate ‘Cash on Delivery’ and ‘Credit Card on Delivery’ payment options. Comment #29 in the issue suggests that a patch could be written to make the core payment module support a dynamic list of payment options, but it is not clear i) whether this is a feature that the UC project would be interested in adding, or ii) whether such a patch would be significantly smaller or less complex than the separate “Credit Card On Delivery” module as currently written. While it would be nice to see more collaboration between the module author and UC maintainers in this instance, we do not feel it would be appropriate to deny the “Credit Card On Delivery” module use case on the basis that it would be unnecessary if Ubercart had capabilities that it currently doesn't.

Regarding the implementation, the module code has room for improvement, such as that identified in comment #29 with respect to database tables. While we agree that the module approval process should involve some mentoring about best Drupal coding practices, it does not appear to us that the module violates any documented Drupal coding guideline or API usage.

Thus, it is the opinion of the Technical Working Group that this module should be considered eligible for promotion to ‘full project’ status, but without granting the ‘git vetted user’ permission to the module author. I am therefore setting the status to RTBC; however, if anyone here disagrees with our assessment of either the use case or the implementation, please feel free to say so and set the issue back to "needs review".

Thanks again for your time, patience, and feedback.

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

Thanks for your contribution, th_tushar!

I promoted this project for you: https://drupal.org/project/credit_card_on_delivery

Now that this experimental project has been promoted, you'll need to update the URL of your remote repository or reclone it.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay 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.

In the Download section of the project page, there should be an Add new release link where you can create a new release based on the development branches (snapshots), or release tags.

If you get an access denied error when logged in and accessing that URL, then the rest of this post may not be useful to you.

You will only be able to create development/snapshot releases from the repository until you push release tags to it.

To fix this, you may want to read the following documentation pages to get a better overall picture of the requirements:

• Types of releases for full projects
• Release naming conventions
• Creating an official release
• A very important note about trying to Fix releases (don't).
• Creating a project release

I've been hacking away at this for a while and I'm pretty sure that there is something wrong with the d.o project <-> git repository association.

The d.o project can only see the branches "master" and "6.x-1.x", and none of the tags. It lists both of those options when choosing the default branch.

The git repository has "6.x-1.x", "7.x-1.x", the associated 1.0 tags to those, plus a brand new "master" branch. (The master was added by me in attempt to see if that would jog it).

From the fact that it can only see 6.x-1.x and master from d.o, and that the master didn't exist in the actual git repo, I'm going to guess that the d.o project is associated with the incorrect repo, or the old sandbox one, or is corrupted. There doesn't appear to be an error with the branches or tags and it should just be working. About the only thing that is different with this repo is that you have the HEAD pointed to a version branch instead of master, but that may just be how things are done these days.

This will need an issue opened on the d.o infrastructure or webmaster queue. Describe the issue and see if they can fix the missing branches and tags.