[REF] Extract self-service eligibility code into its own function

[MegaphoneJon]

Overview

There are two pages that check for self-service eligibility - the "update" and "transfer" pages. The code to check if an event is eligible for cancellation/transfer is a) out-of-sync, b) buggy. This initial PR seeks to extract to a shared function with tests so further improvements can be made.

Before

• Copy/paste code
• No tests
• Self-service transfer doesn't respect the Cancellation or transfer time limit (hours) UI option.

After

• Copy/paste annihilated
• Tests
• Self-service transfer respects the same settings that self-service cancellation does.

Technical Details

If you're reviewing by replicating my extraction, work from CRM_Event_Form_SelfSvcUpdate, since CRM/Event/Form/SelfSvcTransfer is missing validations that the first class doesn't. Once you're comfortable with the extraction, it should be clear what the difference is.

Comments

There are further bugs to fix and improvements to make - event#34 and event#35 but this feels like an important prerequisite.

Additionally, once this is merged I intend to open a PR allowing users to cancel/transfer from their user dashboard.

[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).

[colemanw]

@MegaphoneJon the code looks good but the commit message is not very descriptive...

[MegaphoneJon]

Oh haha. Fixed now.

[eileenmcnaughton]

@MegaphoneJon I think this is a good refactor to do - however, I'm not sold on moving it onto the participant bao - these seem like clearly form-layer functions - as evidenced by the presence of a statusBounce in them.

There is no shared parent at the moment but these forms would lend themself to having a shared trait.

Potentially we would also relocate the code (in a later PR) to being a core-extension. This is a new thing where we can separate features out within core to be packaged as a feature - without any long term plan to move them out of core (but in some cases that might happen down the track)

[MegaphoneJon]

Hi @eileenmcnaughton - I agree with your sentiments about moving this into the BAO; however, this is only an initial PR, which is meant to be a straight refactor plus tests. After it's accepted and under testing, I intend to make further changes:

• It should return a true/false with error message on false, rather than a status bounce. I'll move the status bounce back to the form.
• There are several places where the logic to display transfer/cancel is completely different. I want to replace these with a call to this function to ensure consistency in the UI.

[mattwire]

@MegaphoneJon @eileenmcnaughton This sounds ok to me as there's a promise to refactor further and it sounds like it's needed! @MegaphoneJon Can you add a docblock to the function and a fixme/todo comment to the affect that it will be further refactored.

[MegaphoneJon]

Thanks @mattwire. The first TODO is the one intended to address the concerns @eileenmcnaughton raises. The second and third are bug fixes I'll do afterward.

[MegaphoneJon]

Would it allay concerns if I added a second commit on this PR with the next round of changes? I wanted to keep it separate so someone could evaluate the extraction separately.

[seamuslee001]

test fail unrelated