progressivetech / net.ourpowerbase.qrcodecheckin

QRCode Checkin allows you to send an email that contains a scanable code to the registered participants for your event.
Other
16 stars 15 forks source link

Issue #6 - Support multiple events #11

Closed johntwyman closed 3 years ago

johntwyman commented 3 years ago

This PR introduces the ability to use QR Codes for multiple events at the same time. I've touched a fair bit of the code so I'll outline the main changes included. I'm really happy to get feedback, etc. I was excited to find this extension and really hope this pull request can help other organisations make use of the extension.

Setting renamed & changed to array of type String The setting for this extension is now named qrcode_events and supports an array of Strings that are stored in serialized JSON format. This results in minor changes in the buildForm and postProcess hook functions; they now have to work with an array of values.

qrcodecheckin_participant_id_for_contact_id now takes contact_id & event_id parameters It now relies on the calling tokenValues hook function to provide both parameters but is otherwise the same.

qrcodecheckin_civicrm_tokens generates tokens for each eligible event The token slugs now append the event ID to the end (eg. qrcodecheckin.qrcode_url_23). The main change here though is the notion of "eligible event". The function only generates tokens for events that:

CRM/Qrcodecheckin/Hook.php updated to use non-deprecated pattern The call to CRM_Utils_Hook::singleton()->invoke() has been tweaked to use the non-deprecated pattern of passing in an array of field names as the first argument, rather than just an integer count of the fields.

civicrm_api3 calls refactored to use Api4 I've used Api4 calls over Api3 calls wherever possible, and used the OOP-style calls in particular.

Updated civix boilerplate I ran civix generate:module ... to ensure the latest civix boilerplate is in place

Converted array(...) to [...] It's a minor but prevalent coding style change that puts the code inline with current CiviCRM convention.

Updated chillerlan/php-qrcode to version 3.4 Version 2.x of the php-qrcode library requires versions of PHP that are EOL. I've updated the version to the 3.x branch which requires PHP 7.2 or higher. The 4.x branch requires PHP 7.4 or higher; I thought a smaller step change to be more sensible.

Documentation updated I've updated README.md and two screenshots to better reflect the changes of this PR.

Changes to info.xml I've updated the version to 2.0.0. While this doesn't strictly introduce breaking changes I still feel like is reasonable.

I've also changed the CiviCRM compatibility to 5.0. I haven't tested this on 4.7 and I suspect the use of Api4 calls pretty much excludes anything earlier than 5.0 anyway.

jmcclelland commented 3 years ago

This is great! I'm thrilled to see this functionality added. It's a big change, so I'll need some time to review and test. Thanks for the contribution.

johntwyman commented 3 years ago

This is great - it looks really solid and excluding all the library and boiler plate changes is not a lot of code changes for a valuable feature.

In addition to the date comment - have you tested the upgrade path? I wonder if a simple Upgrade script would be helpful that would simply convert the existing Int variale to an array?

Hey Jamie, thanks for looking over this. I haven't tested the upgrade path and that's a really good pick up. I'll put some time into sorting out an upgrader to ensure that happens smoothly. Should be trivial I imagine (famous last words ;-) ).

As for the dates, the code as written (ie. generating tokens for events based on a future start date) does mean people can send emails out in advance of the events. So changing to future end date doesn't introduce that ability.

If we use end date instead, I can foresee that including events with NULL end dates could lead to lots of tokens "polluting" the token list for what are really old events. Events with start dates five years ago but no end date would be included, for example.

If we only focus on "future end date" (ie. non-NULL) there are two consequences I can think of. First, we'd be able to send emails with QR codes after an event has started. I can only imagine this being useful for events that span multiple days; not a scenario I'm used to.

Second, unfortunately, would be any event without an end date would be missed. That I think is more problematic.

What do you think Jamie? @seamuslee001 tagging you on this given your experience. Do you think future start date or future end date is more sensible?

jmcclelland commented 3 years ago

Oh, you're right about my date comment - I had things backwards. So yes, I think your code as written will accoodate 90% of the use cases.

I originally wrote this extension for the Left Forum, which has a 3 day conference. So for them, being able to send out QR codes for events that have started is useful (they would send a day before, first day, second day and third day email to all participants so the qrcode is at the top of their email box if they are just coming for that day).

I still would lean toward using the end date for these reasons:

johntwyman commented 3 years ago

Ok @jmcclelland I've added an upgrade script and modified the API4 call to retrieve qualifying events on the basis of their end date rather than their start date.

I've tested the upgrade script locally; it looks good. Specifically, I:

  1. Clean installed v1.0.3 of the extension
  2. Enabled QR codes for an event
  3. Inspected the civicrm_setting table to confirm the setting was in the DB
  4. Upgraded the extension to v2.0.0
  5. Inspected the civicrm_setting table to confirm the new setting key existed with an array value
  6. Loaded the manage event page for the event and confirmed its QR checkbox was ticked

I also ran through creating an email to send to registered participants of an event. The token was still generated, so "end date is NULL or in the future" is doing the trick.

My last question for you is whether or not you think restricting this functionality only to events with online registration. It feels wise to me; but then every event we create is designed to let people register themselves online. Have you seen situations where event registration is 100% "back office"? And could those types of events make use of QR codes?

Should we be removing the ->addWhere('is_online_registration', '=', TRUE) line from the \Civi]Api4\Event::get() call?

jmcclelland commented 3 years ago

Thanks for this work - I think this PR is really looking good.

As for your question, I think that we should allow tokens to be created for events without online registration for a few reasons:

  1. The event still needs to be enabled for qr code usage - so it should not result in any new tokens that aren't intended to be created
  2. It seems like users would be able to enable qr codes for an event without online registration - and then, those tokens would simply not appear for reasons that would be quite confusing to the user
johntwyman commented 3 years ago

Cool. That's updated. So I think the PR is now ready. Over to you @jmcclelland :)

jmcclelland commented 3 years ago

Great work!! Everything tested without incident. Thanks for the contribution.

agileware-justin commented 3 years ago

Great job @jmcclelland @johntwyman - really needed this functionality to use this extension