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

EVA-157 Conditional Endpoint Disabling #2101

Conversation

pattihis
Copy link
Contributor

@pattihis pattihis commented Jun 8, 2024

🎫 Ticket

EVA-157
EVA-158

🗒️ Description

  1. We were checking the dependencies of 'create_events' endpoint only. I added the rest, checking in two groups, for TEC and ET dependant actions. There was also one filter that did not exist any more, about queues, so I adjusted the code there as well.
  2. The tooltips were not showing because the assets of EVA were not loading properly after the merge so I've fixed that too.

✔️ Checklist

  • Changelog entry in the readme.txt file.
  • Code is covered by NEW wpunit or integration tests.
  • Code is covered by EXISTING wpunit or integration tests.
  • Are all the required tests passing?
  • Automated code review comments are addressed.
  • Have you added Artifacts?
  • Check the base branch for your PR.
  • Add your PR to the project board for the release.

src/admin-views/dashboard/endpoints/endpoint.php Outdated Show resolved Hide resolved
src/admin-views/dashboard/endpoints/endpoint.php Outdated Show resolved Hide resolved
src/admin-views/dashboard/endpoints/endpoint.php Outdated Show resolved Hide resolved
src/admin-views/dashboard/endpoints/endpoint.php Outdated Show resolved Hide resolved
src/admin-views/dashboard/endpoints/endpoint.php Outdated Show resolved Hide resolved
src/admin-views/zapier/dashboard/endpoints/endpoint.php Outdated Show resolved Hide resolved
src/admin-views/zapier/dashboard/endpoints/endpoint.php Outdated Show resolved Hide resolved
src/admin-views/zapier/dashboard/endpoints/endpoint.php Outdated Show resolved Hide resolved
src/admin-views/zapier/dashboard/endpoints/endpoint.php Outdated Show resolved Hide resolved
src/admin-views/zapier/dashboard/endpoints/endpoint.php Outdated Show resolved Hide resolved
@pattihis pattihis self-assigned this Jun 9, 2024
@pattihis pattihis added code review Status: requires a code review. merge Status: ready to merge. labels Jun 9, 2024
*
* @since TBD Migrated to Common from Event Automator
*
* @param array<string,array> $endpoint An array of the integration endpoint details.
* @param Abstract_REST_Endpoint $this An instance of the endpoint.
*/
return apply_filters( "tec_event_automator_{$api_id}_queue_endpoint_details", $endpoint, $this );
return apply_filters( "tec_event_automator_{$api_id}_endpoint_details", $endpoint, $this );
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to deprecate the old filter in case of 3rd party usage? cc @jesseeproductions

Copy link
Contributor

Choose a reason for hiding this comment

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

Being that the @since is TBD doesn't that mean this is a new filter. Not in production, yet?

Copy link
Contributor

@codingmusician codingmusician left a comment

Choose a reason for hiding this comment

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

Good stuff. A few nit picky things I saw.

Comment on lines +699 to +700
* @param WP_REST_Server $handler ResponseHandler instance (usually WP_REST_Server).
* @param WP_REST_Request $request Request used to generate the response.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit pick:

Suggested change
* @param WP_REST_Server $handler ResponseHandler instance (usually WP_REST_Server).
* @param WP_REST_Request $request Request used to generate the response.
* @param WP_REST_Server $handler ResponseHandler instance (usually WP_REST_Server).
* @param WP_REST_Request $request Request used to generate the response.

$this->template(
'components/read-only',
[
'classes_wrap' => [ 'tec-automator-grid-item', 'tec-settings-connection-endpoint-dashboard-details__name-wrap', ! $endpoint['enabled'] || $endpoint['missing_dependency'] ? 'disabled' : '' ],
Copy link
Contributor

Choose a reason for hiding this comment

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

This is super long. It might be better to do this:

Suggested change
'classes_wrap' => [ 'tec-automator-grid-item', 'tec-settings-connection-endpoint-dashboard-details__name-wrap', ! $endpoint['enabled'] || $endpoint['missing_dependency'] ? 'disabled' : '' ],
'classes_wrap' => [
'tec-automator-grid-item',
'tec-settings-connection-endpoint-dashboard-details__name-wrap',
! $endpoint['enabled'] || $endpoint['missing_dependency'] ? 'disabled' : '',
],

Comment on lines +98 to +124
if ( $endpoint['missing_dependency'] ) {
$this->template(
'dashboard/components/missing-dependency',
[
'endpoint' => $endpoint,
'manager' => $manager,
'url' => $url,
] );
}
]
);
} else {
$this->template(
'dashboard/components/clear-button',
[
'endpoint' => $endpoint,
'manager' => $manager,
'url' => $url,
]
);
$this->template(
'dashboard/components/status-button',
[
'endpoint' => $endpoint,
'manager' => $manager,
'url' => $url,
]
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're sending the same args into the template, it might be simpler to store them in a variable to pass them along:

$template_args = [
	'endpoint' => $endpoint,
	'manager'  => $manager,
	'url'      => $url,
];

That way, the template lines are on one line:

$this->template( 'dashboard/components/missing-dependency', $template_args );

Comment on lines +90 to +116
if ( $endpoint['missing_dependency'] ) {
$this->template(
'zapier/dashboard/components/missing-dependency',
[
'endpoint' => $endpoint,
'manager' => $manager,
'url' => $url,
] );
}
]
);
} else {
$this->template(
'zapier/dashboard/components/clear-button',
[
'endpoint' => $endpoint,
'manager' => $manager,
'url' => $url,
]
);
$this->template(
'zapier/dashboard/components/status-button',
[
'endpoint' => $endpoint,
'manager' => $manager,
'url' => $url,
]
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here from above about putting the template args into a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codingmusician thanks. To give you some context, this is a file from EVA that was merged in Common recently. I did not make these changes; all these additions you see are because of the PHPCS formating to satisfy the @tec-bot review.

I had different commits for my changes and for the code formatting to help the reviewers.

The actual change in this file was at line 97 of the original, where it had a
if ( $endpoint['missing_dependency'] ) {

I moved that higher to also exclude the action buttons from showing. Everything else is from the phpcbf.

@pattihis pattihis removed code review Status: requires a code review. merge Status: ready to merge. labels Jun 12, 2024
@pattihis pattihis merged commit e1ded06 into bucket/plugin-consolidation Jun 12, 2024
11 of 16 checks passed
@pattihis pattihis deleted the fix/EVA-157-Conditional-Endpoint-Disabling branch June 12, 2024 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants