Add 'Re-apply promotions' option to the attendee mover confirmation step

[Pebblo]

I don't consider this finished yet, but I mentioned it in Slack and Brent offered to take a look and give feedback so here goes. I'll add details in this PR as the other one related to this in core is a single change to add a method to the service and its for functionality in this add-on.

Most of this seems fine, adding the option etc I don't have a problem with, however, the call to the new copyPromotionLineItems() method feels a little fragile and I'm not sure what the best way to avoid/reduce that is.

The call relies on the promotions add-on and I know I could just use class_exists() to make sure the promotions add-on is available before running it (including doing so within the method itself) or even move that method away from using functions in the promotions add-on directly but I get the feeling the service should be added from within the promotions add-on itself rather than patching it into core, right?

I don't know how the best way to go about doing that so any pointers appreciated :)

Would I need to add dependency injection to the promotions add-on and add a /domain/services/line_items/ service for example or something completely different?

Problem this Pull Request solves

It applies the promotion set on the 'original' registration to the 'new' one, without incrementing the 'usage' value of that add-on (as the original usage would have done so).

The changes in this specific PR don't do much other than add the visuals for that and the option not to apply promotions.

How has this been tested

Register onto an event and use a promotion during checkout for whatever value/percentage. Finalize.

Move the registration onto anther ticket/event and select to re-apply promotions, the 'new' registration should have the promotion applied.

(Note this intentionally skips any/all checks for promotion being valid etc and just applies it)

Checklist

• [ ] I have read the documentation relating to systems affected by this pull request, see https://github.com/eventespresso/event-espresso-core/tree/master/docs
• [ ] User input is adequately validated and sanitized
• [ ] all publicly displayed strings are internationalized (usually using esc_html__(), see https://codex.wordpress.org/I18n_for_WordPress_Developers)
• [ ] My code is tested.
• [ ] My code follows the Event Espresso code style.
• [ ] My code has proper inline documentation.

[tn3rb]

everything looks great so far well... ...except for the things that I know you copied from code I wrote that I would likely do differently now 😞

Now with regards to simply hardcoding content specific to one add-on into code for another add-on (or into core), that's a big nope, but is also relatively easy to fix. We just need to add some filters and actions here and there. That's not a big deal because users would have to download and update to the new versions that contain your new hardcoded changes, so they may as well do so with the versions that just have some hooks added.

So this is what we do:

• EventEspresso\AttendeeMover\form\Step.php needs to modify the redirect and form action args. $form_args is a private property on \EventEspresso\core\libraries\form_sections\form_handlers\FormHandler and $redirect_args is a private property on \EventEspresso\core\libraries\form_sections\form_handlers\SequentialStepForm. We can add filters for both of those properties within their setters. The promotions add-on can add its value for ee-copy-promos via those filters.

• then in \EventEspresso\AttendeeMover\form\VerifyChanges::generate() you simply need to filter $form prior to passing it to setForm(). This will allow the promotions add-on to inject it's new subsection.

• \EventEspresso\AttendeeMover\form\VerifyChanges::process() also needs to filter the redirect args to inject the value for ee-copy-promos, but it can do so via the same filter we used earlier.

• things get trickier when wanting to add the value for copy_promotions to services/commands/MoveAttendeeCommand.php since commands were not initially intended to be extended like that but obviously there is a need for it. There's a couple of ways we can do this, but they all most likely start with:

  • adding a private array property to \EventEspresso\core\services\commands\Command called something like extra_properties.

  • add a public setter and getter, ie: setExtraProperties() and getExtraProperties()

  • then we either:

    • filter the MoveAttendeeCommand object within \EventEspresso\AttendeeMover\form\Complete::process() just prior to passing it to the Command Bus

    • or use the AHEE__EventEspresso_core_services_commands_middleware_AddActionHook__handle__before which runs automagically prior to command execution. (there's also AHEE__EventEspresso_core_services_commands_middleware_AddActionHook__handle__after that runs afterwards). The callback for this simply needs to check that the supplied command is an instance of MoveAttendeeCommand before doing its thing.

  • either of those hooks can be used to call setExtraProperties() and add a value for copy_promotions

• then finally, for running the logic that copyPromotionLineItems() does, we simply need another action. I would add one right at the end of \EventEspresso\core\domain\services\registration\CopyRegistrationService::copyRegistrationDetails()

And that's it... at least for the code in this PR. I realize there's some other code that's not part of this PR (like whatever's in copyPromotionLineItems()) but I can't really comment on that without seeing any of it.

re:

Would I need to add dependency injection to the promotions add-on and add a /domain/services/line_items/ service for example or something completely different?

ya... really hard to say without seeing what you are trying to do.

Dependency Injection is simply the preferred way to make class A available to class B because class A's logic is required for class B to function. So if you create a class in the promotions add-on that depends on another class (or modify an existing one and add a dependency), then this is what you need to do:

• in \EE_Promotions::after_registration() add a call to EE_Dependency_Map::register_dependencies() and pass the array of dependencies for your class. You can see examples of this in \EE_Attendee_Mover::after_registration() but plz note that I think you would likely also need to declare a namespace for the promotions addon, since I believe it was written while we still supported PHP 5.2. I can help you with that as well if need be.

• then once your dependencies are registered, you can load the class from anywhere using \EventEspresso\core\services\loaders\Loader() either via

  • getNew() if you require a brand new unique instance of your class or
  • getShared() if you only want there to be one instance of your class used everywhere

• the dependencies you registered earlier will be automagically injected into your class for you. There's some edge cases that get a bit weird when doing this but 99% of the time is super simple and you'll never want to do things differently.

re:

a /domain/services/line_items/ service

• despite it already being a horrible monstrosity of a file, it would likely be easiest to add your hooks into \EED_Promotions::set_hooks() or \EED_Promotions::set_hooks_admin() and THEN use the loader mentioned above (which you can magically instantiate anywhere using \EventEspresso\core\services\loaders\LoaderFactory::getLoader()) and then load up your new class that contains all of your new logic. This is how I like to use modules now. They are the low tech integration with WordPress that are used to load your more complex code.

Anyways, hope that all helps. Do not hesitate to hit me up in chat to explain things or pass this back with more questions.

One last point, when you create a PR, there is an option to designate it as a Work In Progress PR which means it will not allow it to be merged to it's parent branch, but still allow you to do everything else, like request code reviews. You can convert it to a real PR later on when you are ready to get serious ;)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically closed because it has been stale for a significant period of time without any activity.