Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract Attendee Info Reg Form Processing into New Classes #3419

Conversation

tn3rb
Copy link
Member

@tn3rb tn3rb commented Jun 3, 2021

This PR:

  • extracts logic from EE_SPCO_Reg_Step_Attendee_Information::process_reg_step() into the following new classes:

    • RegFormHandler
    • RegFormInputHandler
    • RegFormAttendeeFactory
    • RegistrantData
  • extracts a bit more logic from the RegForm into PrivacyConsentCheckboxForm

  • extracts most of the dependency registration from EE_Dependency_Map into RegFormDependencyHandler

  • refactors a few other form related things after the above changes

  • cleans up a few other things

Comment on lines -805 to +806
'EventEspresso\core\domain\services\registration\form\v1\RegForm' => [
null,
'EE_Registration_Config' => EE_Dependency_Map::load_from_cache,
],
'EventEspresso\core\domain\services\registration\form\v1\RegistrationForm' => [
null,
null,
null,
null,
'EEM_Event_Question_Group' => EE_Dependency_Map::load_from_cache,
],
'EventEspresso\core\domain\services\registration\form\v1\QuestionGroupRegForm' => [
null,
null,
null,
'EventEspresso\core\domain\services\registration\form\v1\RegFormQuestionFactory' => EE_Dependency_Map::load_from_cache,
],
'EventEspresso\core\domain\services\registration\form\v1\CountryOptions' => [
null,
'EEM_Answer' => EE_Dependency_Map::load_from_cache,
'EEM_Country' => EE_Dependency_Map::load_from_cache,
],
'EventEspresso\core\domain\services\registration\form\v1\StateOptions' => [
null,
'EEM_State' => EE_Dependency_Map::load_from_cache,
'EventEspresso\core\domain\services\registration\form\v1\RegFormDependencyHandler' => [
'EE_Dependency_Map' => EE_Dependency_Map::load_from_cache,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

$reg_form = $this->checkout->current_step->reg_form;
$required_questions = $reg_form instanceof RegForm ? $reg_form->requiredQuestions() : [];
$this->input_handler = LoaderFactory::getShared(
RegFormInputHandler::class,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now you like ::class 😄

Comment on lines +277 to +278
$copy_primary = isset($reg_form_data[ $reg_url_link ]['additional_attendee_reg_info'])
&& absint($reg_form_data[ $reg_url_link ]['additional_attendee_reg_info']) === 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply this?

Suggested change
$copy_primary = isset($reg_form_data[ $reg_url_link ]['additional_attendee_reg_info'])
&& absint($reg_form_data[ $reg_url_link ]['additional_attendee_reg_info']) === 0;
$copy_primary = empty($reg_form_data[ $reg_url_link ]['additional_attendee_reg_info']);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm guessing (since that code was probably originally written close to a decade ago) that additional_attendee_reg_info might hold value with different data types... like bool, int, string so running absint() on a string value (and then comparing against 0) is very different than just using empty().

what you've suggested might very well work fine, but i'm really hesitant to modify the logic in older code if I don't have to because there may be some weird edge case that some seemingly unnecessary line of code is accounting for. I've tried really hard to keep things operating exactly the same throughout this refactor because I don't want the existing code to get broken :\

Copy link
Contributor

@manzoorwanijk manzoorwanijk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 🚀

Copy link
Member

@knazart knazart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants