This module provides tools to use Drupal Commerce as a powerful event booking system. Any entity can be an 'event' and a single booking can contain any number of tickets.

Commerce Orders are used as "Bookings" and we create a new fieldable entity type "Commerce Booking Ticket" to model the tickets for an event. A booking manager can add any number of tickets to their booking.

A number of payment options are available out of the box. Booking managers can choose to pay just the deposits needed to book tickets, the full price of the tickets or a custom amount for each ticket anywhere in between.

With the Commerce Booking Flexible Pricing module enabled administrators can define ticket classes (such as 'Concessions') and Booking Windows (e.g, 'Early Bird'). Ticket Classes are rules condition sets and are chosen for a ticket automatically based on the conditions defined by the rule. Booking Windows consist of a Label and Cut Off Date, after which the window has expired. This allows event editors to set complex pricing structures for their events.

Views integration is included in the module and it is left to the SiteBuilder to add and configure views to give helpful reports to the users.

Project Page: http://drupal.org/sandbox/rlmumford/1800812
Git Repo: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/rlmumford/1800812.git

Review Bonus
http://drupal.org/node/1873786#comment-7237886
http://drupal.org/node/1937044#comment-7237982
http://drupal.org/node/1955522#comment-7238118
http://drupal.org/node/1957554#comment-7272200
http://drupal.org/node/1964596#comment-7272360
http://drupal.org/node/1964532#comment-7272690

Comments

ledzepp94’s picture

ledzepp94 commented
Status: Needs review » Needs work

It looks great, only a few errors popping up through a coder review:

http://ventral.org/pareview/httpgitdrupalorgsandboxrlmumford1800812git

for example:

FILE: ...pareview_temp/views/handlers/commerce_booking_ticket_ticket_balance.inc
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
7 | ERROR | Class name must begin with a capital letter
7 | ERROR | Class name must use UpperCamel naming without underscores
12 | ERROR | No scope modifier specified for function "construct"
21 | ERROR | No scope modifier specified for function "query"
29 | ERROR | No scope modifier specified for function "render"
--------------------------------------------------------------------------------

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Status: Needs work » Needs review

Thanks for the review!

Yeah I noticed those. I'm not sure whether to fix them or not.

The issues you've highlighted in that comment is because the class extends views_handler_field which doesn't obey coding standards itself.

The issue in rules_config.entity_reference.inc is because its using a pattern started by the entity reference module.

The issue in commerce_booking_ticket.entity.inc is there because the database column the property represents is the 'ticket_id' column.

Maybe someone who knows can tell me whether what I've done with these is correct?

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Issue tags: +PAreview: review bonus

Adding the review bonus tag

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

manual review:

  1. commerce_booking_permission(): why do you need to check_plain() the type name? Isn't that a machine name restricted to alphanumeric characters?
  2. "module_invoke_all('commerce_booking_ticket_status_info')": that hook is not documented in the api.php file?
  3. commerce_booking_commerce_booking_ticket_status_info(): why are the labels and descriptions not sent through t() for translation? Please add a comment.

need to run now, will continue later.

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented

Hi klausi! Thanks for the review. I look forward to part 2 =)

why do you need to check_plain() the type name? Isn't that a machine name restricted to alphanumeric characters?

I had thought It was worth the extra check as we don't necessarily know how the value got into the database, but if an invalid value has got into the commerce_booking_ticket_types table then we have problems already.

I've added documentation for the two hooks and wrapped the status name and descriptions in t(). I've also updated the docs for some functions further down.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: Unassigned » sreynen
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

Quite a complex module, but all looks good to me from a visual code review, so I would say RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to sreynen as he might have time to take a final look at this.

sreynen’s picture

sreynen commented
Status: Reviewed & tested by the community » Needs work

Enabling Commerce Booking Flexible Pricing throws an error for me:

PDOException: SQLSTATE[HY000]: General error: 1364 Field 'plugin' doesn't have a default value: INSERT INTO {rules_config} (name, status, module) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => commerce_booking_standard [:db_insert_placeholder_1] => 2 [:db_insert_placeholder_2] => commerce_booking_flexible_pricing ) in drupal_write_record() (line 7136 of /includes/common.inc).

Adding a booking field to a content type and attempting to complete an order throws these errors:

Notice: Undefined index: deposit_field in commerce_booking_checkout_pane_ticket_payments_checkout_form() (line 347 of /sites/default/modules/1800812/commerce_booking.checkout.inc).
EntityMetadataWrapperException: Unknown data property . in EntityStructureWrapper->getPropertyInfo() (line 339 of /sites/all/modules/entity/includes/entity.wrapper.inc).

I don't know what's happening with the install error, but the second error looks like maybe the field configuration form has required fields that are not marked as required. I don't like to move this back to "needs work" but these seem like big enough bugs that they need to be fixed before you release this.

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Status: Needs work » Needs review

Thanks for catching those two. I've fixed them now so am setting back to needs review.

I've also done another documentation check and run it through the code sniffer.

yanniboi’s picture

yanniboi commented
Status: Needs review » Needs work

I installed Commerce booking and flexible pricing fields with no errors. Fix must have worked :)

I tested making a node an event using the commerce_booking fields and this works well. The instructions in the README file are clear and helpful.

No errors found!

yanniboi’s picture

yanniboi commented
Status: Needs work » Needs review

No idea why I set it to needs work, that is wrong... Apologies.

yanniboi’s picture

yanniboi commented
Issue summary: View changes

Updated issue summary.

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Issue summary: View changes

Added a review

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Issue summary: View changes

Added review.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

So back to RTBC I guess, care to take another look sreynen?

sreynen’s picture

sreynen commented
Status: Reviewed & tested by the community » Needs work

I've followed, to the best of my ability, the instructions in the README.txt, and I'm still getting a fatal error on checkout:

EntityMetadataWrapperException: Unknown data property _none. in EntityStructureWrapper->getPropertyInfo() (line 339 of /home/s1154992c81f62de/www/sites/all/modules/entity/includes/entity.wrapper.inc).

The instructions are confusing, so maybe I've missed something. For example, "Ensure the price field on your event entity is set to 'Unlimited' cardinality." comes before any mention that I need a price field at all. If this is required, as it seems to be, it should be marked as required in the commerce booking field options.

Also, using no terms and conditions, the default option, shows an empty terms and conditions that must be agreed to. The confusing instructions and terms and conditions thing are not blockers, though I suspect they're related to a general problem of this not being tested enough with default options.

I do think I need to be able to complete a checkout without a fatal error before this has a release, so I'm moving it back to needs work. I'm sure there's some configuration I could do to avoid errors, but it's not clear to me what that configuration is, and ideally it wouldn't be possible to trigger errors at all. It certainly shouldn't be as easy as using only default options.

rlmumford’s picture

rlmumford
Primary language English
Location Manchester
commented
Status: Needs work » Needs review
Issue tags: +PAreview: review bonus

I've updated the documentation in the README, so hopefully those instructions are clearer now. Please let me know if you have any suggestions for further improvement.

The booking process now skips the Terms&Conditions step if no terms and conditions exist.
The booking process works without a Deposit field set.
The 'Price Field' and 'Date Field' fields are now marked as required on the Commerce Booking field settings form.
The Flexible Pricing stuff works without a booking windows field (and with a booking windows field).

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

@sreynen: kudos to you for testing so thoroughly! Nevertheless I think we can proceed here and I don't want to hold up a project just because there are bugs that stem from missing configuration/documentation. That can all be handled as follow-ups in the issue queue of the module.

Any other security concerns?

sreynen’s picture

sreynen commented
Status: Reviewed & tested by the community » Fixed

Nope, I don't see any other issues, and rlmumford has demonstrated an ability to respond to issues, so I'm not really concerned about remaining issues.

Thanks for your contribution, rlmumford!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

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

yautja_cetanu’s picture

yautja_cetanu commented

I work with rlmumford.

Thanks a lot sreynen and klausi for your help with all this :D Its been a lot of fun getting stuck into the community even more. Your feedback has definitely been appreciated :D!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Added Review