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

feat: QL Events settings tab added and schema organized #55

Merged
merged 41 commits into from
May 26, 2023

Conversation

kidunot89
Copy link
Collaborator

@kidunot89 kidunot89 commented Apr 24, 2023

Summary

  • Adds the QL Events settings tab.
  • Restructures schema so missing dependency plugins not required by the schema won't cause an error.
  • Fixes a TicketField bugs related the child field types and adds unit tests for all ticketFields queries.

@kidunot89 kidunot89 requested a review from bordoni April 24, 2023 23:49
Copy link
Member

@bordoni bordoni left a comment

Choose a reason for hiding this comment

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

Two small comments, and double checking why are we using so much Anonymous functions? Instead of creating a new method, for example on ql_events_resolve_tec_order_type and so many others. It's ok, just wanted to understand why.

Comment on lines 22 to 24
public function __construct() {
add_action( 'graphql_register_settings', [ $this, 'register_settings' ] );
}
Copy link
Member

Choose a reason for hiding this comment

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

Not a required change, but I would love to avoid adding filters on a construct, it prevents a misfire of double hooking if we move it to a hook() method

* @return array
*/
public static function get_fields() {
$test_mode_status = (bool) defined( 'QL_EVENTS_TEST_MODE' ) && QL_EVENTS_TEST_MODE ? 'force enabled' : 'force disabled';
Copy link
Member

Choose a reason for hiding this comment

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

I wounder if we should have a method for the test mode verification.

@kidunot89
Copy link
Collaborator Author

Two small comments, and double checking why are we using so much Anonymous functions? Instead of creating a new method, for example on ql_events_resolve_tec_order_type and so many others. It's ok, just wanted to understand why.

Cuz I was rushing lol

@kidunot89 kidunot89 requested a review from bordoni April 27, 2023 23:04
Copy link
Member

@bordoni bordoni left a comment

Choose a reason for hiding this comment

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

Minor things and some questions, would love to see us having @since TBD in general.

And replace all TBDs right before launch with the version.

Comment on lines +161 to +182
'resolve' => function( $source ) {
$decorator = tribe( Attendee::class );
$decorated_attendee = $decorator->get_attendee( get_post( $source->ID ) );

$meta = tribe( 'tickets-plus.meta' );
$attendee_meta_data = $meta->get_attendee_meta_fields( $decorated_attendee->ticket_id, $decorated_attendee->ID );
if ( isset( $attendee_meta_data[0] ) ) {
unset( $attendee_meta_data[0] );
}

if ( ! is_array( $attendee_meta_data ) ) {
return [];
}

return array_map(
function( $key, $value ) {
return (object) compact( 'value', 'key' );
},
array_keys( $attendee_meta_data ),
array_values( $attendee_meta_data )
);
},
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but I would love to see us move towards having the actual method live in this class, and we pass it as a Callable here. Feels like it would be better for long term unit testing and readability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What method are you referring to?

Copy link
Member

Choose a reason for hiding this comment

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

To the actual function that is assigned to the 'resolve' key, I would love for us to go towards using methods from this class, instead of anonymous functions. Which will make the code harder to test and read down the line.

includes/types/object/class-wooattendee-type.php Outdated Show resolved Hide resolved
default:
$type = apply_filters( 'ql_events_resolve_ticket_type', null, $value );
Copy link
Member

Choose a reason for hiding this comment

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

Docblock missing here.

default:
$type = apply_filters( 'ql_events_resolve_tec_order_type', null, $value );
Copy link
Member

Choose a reason for hiding this comment

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

Docblock missing here.

default:
$type = apply_filters( 'ql_events_resolve_attendee_type', null, $value );
Copy link
Member

Choose a reason for hiding this comment

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

Docblock missing here.

Comment on lines +38 to +39
*
* @param array $ticket_classes - TEC ticket class names.
Copy link
Member

Choose a reason for hiding this comment

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

It would be cool to see some @since TBD in the docblocks.

kidunot89 and others added 2 commits May 26, 2023 00:17
@kidunot89 kidunot89 requested a review from bordoni May 26, 2023 05:17
@kidunot89 kidunot89 merged commit 0f04299 into master May 26, 2023
@kidunot89 kidunot89 deleted the feat/settings-tab branch July 26, 2023 19:35
@kidunot89 kidunot89 added the enhancement New feature or request label Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants