event#34: allow negative self-service/transfer time

[MegaphoneJon]

https://lab.civicrm.org/dev/event/-/issues/34

Overview

Events are often multi-day events; e.g. if events represent university courses, and there is a period wherein students may freely cancel their registration in the first week of classes, it's desirable to have the cancel time be negative.

Before

Entering a negative value into "Cancellation or transfer time limit (hours)" results in a DB Error.

After

Entering a negative value into "Cancellation or transfer time limit (hours)" saves correctly and works as expected.

Technical Notes

civicrm_event.selfservicexfer_time is now an int and not an unsigned int.

Comments

I regenerated the DAO, but I'm not sure if that's necessary here, because the DAO doesn't differentiate between the unsigned and signed int?

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@MegaphoneJon test fails relate (which is more good than bad)

CRM_Event_BAO_ParticipantTest::testGetSelfServiceEligibility with data set #1 (1, 12, 8, 1, false, false)
Undefined variable: 12

/home/jenkins/bknix-dfl/build/core-18067-644q1/web/sites/all/modules/civicrm/CRM/Event/BAO/Participant.php:1933
/home/jenkins/bknix-dfl/build/core-18067-644q1/web/sites/all/modules/civicrm/tests/phpunit/CRM/Event/BAO/ParticipantTest.php:437
/home/jenkins/bknix-dfl/build/core-18067-644q1/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:209
/home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar:615

[MegaphoneJon]

Thanks for the heads-up Eileen! Once I added this patch, I remembered why I made the $deadlineIsNegative variable - but decided to rewrite it since I don't think it added much clarity. But that's how typos happen...

[eileenmcnaughton]

From a code point of view this looks very tidy - @jitendrapurohit any chance you can test? I think you & @MegaphoneJon are the only 2 people who have ever shown any interest in this functionality

[eileenmcnaughton]

@MegaphoneJon sorry needs another rebase

[MegaphoneJon]

A question about that - the DAO doesn't distinguish between int and unsigned int - so maybe I shouldn't be running regen.sh at all. Does that make sense?

It wouldn't avoid this rebase, but it would keep me safe from other DAO changes causing a future rebase, and would be good info to have in any case.

[eileenmcnaughton]

@MegaphoneJon that appears to be the case.... (@seamuslee001 )

[eileenmcnaughton]

@petednz - @jitendrapurohit am hoping I can get Fuzion to review this one....

[jitendrapurohit]

think it missed the above/first ping. I'll review this today.

[eileenmcnaughton]

thanks @jitendrapurohit - I don't think many other people have looked at this code

[jitendrapurohit]

Tested and confirmed that it works fine in calculating the deadline and displaying the appropriate msg on the screen. The msg sent via email looks a bit off -

You may transfer your registration to another participant or cancel your registration up to -4 hours before the event. Cancellations are not refundable.

These are hardcoded in the message templates so would need to be another "upgrade handling" part. Not sure if required though.

I had to apply the alter statement from the upgrade file in order to test, but think that will be handled by the regen automatically.

Probably we should also extend the help text to document the purpose of negative value?

[MegaphoneJon]

Thanks for your review Jitendra, and these were all good catches!

Tested and confirmed that it works fine in calculating the deadline and displaying the appropriate msg on the screen. The msg sent via email looks a bit off -

You may transfer your registration to another participant or cancel your registration up to -4 hours before the event. Cancellations are not refundable.

This is fixed now.

I had to apply the alter statement from the upgrade file in order to test, but think that will be handled by the regen automatically.

This concerned me, so I took a look - this needed a fix, since we're now targeting 5.30 and now 5.29.

Probably we should also extend the help text to document the purpose of negative value?

This is also fixed.

[eileenmcnaughton]

@jitendrapurohit good to merge now?

[jitendrapurohit]

Thanks for the quick update @MegaphoneJon. I've confirmed that the above points are sorted. Pls merge @eileenmcnaughton.

[eileenmcnaughton]

Thanks @MegaphoneJon , thanks @jitendrapurohit