bocalig [bocalig]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

I am not an official CVS reviewer however I needed a module like this and decided to give it a test run.

First off this is a good use case for a module that could easily make it into core ubercart or Drupal Commerce after it proves itself in the contrib space.

The module has a good plan by creating a custom block, however the module fails on execution and coding standards in many ways.

This is in no way a complete list but just a few things I noticed that must be fixed before a full review can take place.

To test this module I used the Uberdrupal Install profile (drupal.org/project/uberdrupal) for a couple of reasons.
1. It installs ubercart and creates a test product
2. It uses a theme that by default uses a content top region
3. This would be a great module to include in the install profile.

The very first thing I did was enable the module and then go to admin/build/blocks to move the block to content top.
I didn't adjust the block settings because i noticed that the module was setting those my default (I looked at the code before installing).

This first thing that happens is not so great. (Screenshot 1)
I didn't expect the block to show up on a non checkout or cart page.
Looking at the block settings I realize there is a mistake in the module, the module sets a path of 'cart/*' but doesn't set the visibility to 1 and there for the block is defaulting to show on every page but the cart and checkout pages. (oops)

The URL should be 'cart' and 'cart/*' as cart/* won't allow the block to show on site.com/cart (step 1).

So i fixed the above issues and added a product to my cart and proceeded to step 1 of viewing the cart. (Screenshot 2)

The first thing that is obvious is broken css, but also the Proceed to checkout link is broke because it doesn't use the l() function.

The only css change needed in Prosper is setting the .block ol margin to 0.

I then went and switched to garland (most module css at least works with core garland right?) (Screenshot 3)

It should be noted I put it into the content region as there is no content-top.

I think there is a bad combination of html markup and css going on here. You have to be careful when you module defines ol and li as many themes assume that these items need margins/padding to make them look like a nice list. I think using ol/li is not the right way to go with this desired result.

There are other minor issues with spacing/white space but the look and function is far more important with this module.

I can't wait to see a revised module to test.

I am adding the tag we are using for Ubercart related modules.

I think the following post might help you out.

http://drupal.org/node/446628

Looks like you need a:

$blocks[0][visibility'] = 1;

I will review this in the morning for my own use of this module I do see that bocalig is requesting to be approved as a co-maintainer of another module, so that will bypass the need for this CVS application.

#947890: Co-Maintainer

This looks from an outsiders perspective to be a workaround to the CVS application as the maintainer of the other module is a co-worker. This happens often, so bocalig is not exactly doing anything wrong, just working the system.

Just a few small comments:

Be aware that there are threads providing similar solutions on ubercart.org - you should check those out and see if there are some good ideas you can incorporate into you module. This isn't a complete list, but a starting point:
http://www.ubercart.org/project/progress checkout block
http://www.ubercart.org/forum/support/7197/6x_dev_order_progress_bar

I don't think that putting all the logic into the theme layer (via an include in the template) is the best way to go. Perhaps you could put the logic into the module and pass a variable or variables to the template containing the information the template needs in order to decide what to display.

You're defining an awful lot of CSS classes - are all these really necessary? It makes it difficult to theme when you have to figure out all those different combinations. The .css file should be named uc_progressbar.css using your module name, and is usually not in its own directory. old-bg.zip should be removed if it's not used by the module.

Also, pay attention to coding standards. Use the Coder module to help.

Hi,

Glad you want to contribute. The code looks good.

1) I would like to see a comparison of the different modules with similar functionality (cited in #10).

2) As suggested, please install and run Coder module and eliminate the problems shown there:
Coder found 1 projects, 3 files, 24 normal warnings

3) All modules should ideally have a README.txt (explaining what it does, how to install [usual stuff], how to administer it, how to use it etc.). See http://drupal.org/node/161085

I haven't looked at the module, but just from reading the thread here, if the aim of this module is to provide a progress bar, this raises a number of questions:

• isn't this a general-purpose function which doesn't need to be related to the Ubercart function?
• if it is tied so closely to Ubercart that it cannot be separated, wouldn't it be better as an Ubercart patch?
• aren't there already progress bar modules? e.g. http://drupal.org/node/213374
• if there are already progress bar module(s), what does this module do that wouldn't be better as part of other modules?

Just questions, wanting to make clear the degree of connection/independence with Ubercart module, and the degree of overlap with other progress bar modules.

Just as a side note, the referenced progress bar module referenced by drupalshrek is a progress bar for something like a file upload, or some other kind of process that might take a while and it's important for the user to have some kind of feedback that there's progress being made.

The progressbar that bocalig is working on is a progress bar for the steps required for checkout:

step 1/4: /cart
step 2/4: /cart/checkout
step 3/4: /cart/review
step 4/4: /cart/complete (success!)

Hope this clears any ambiguity up.

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.