Skip to content

Conversation

@saifsultanc
Copy link
Contributor

Context

⛑️ Ticket(s): https://secure.helpscout.net/conversation/2115235612/42388?folderId=7098280

Summary

Add a field other than the Date field to the Notification Scheduler. In this instance, a "Single Line Text Field" was used.

@github-actions
Copy link

github-actions bot commented Feb 10, 2023

Warnings
⚠️ When ready, don't forget to request reviews on this pull request from your fellow wizards.

Generated by 🚫 dangerJS against aecfdc3

Copy link
Contributor

@claygriffiths claygriffiths left a comment

Choose a reason for hiding this comment

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

LGTM! Just be sure to fix up the PHPCS issues.

I'd also go with something like this for the commit message:

gpns-add-other-fields-to-date-schedule.php: Added snippet to add a specific field ID to be treated as a date field for scheduling.

Comment on lines +15 to +16
// add the target field to fields array
array_push( $fields, $field );
Copy link
Contributor

Choose a reason for hiding this comment

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

Would $fields[] = $field; work here?

Copy link
Contributor

@scyt scyt left a comment

Choose a reason for hiding this comment

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

Approved with Clay's requested changes.

@saifsultanc saifsultanc force-pushed the saif/add/42388-add-other-fields-to-gpns branch from 3d9d0b8 to 79892bf Compare January 10, 2025 11:49
@saifsultanc saifsultanc changed the title gpns-add-other-fields-to-date-schedule.php: Add other fields to the GP Notification Scheduler. gpns-add-other-fields-to-date-schedule.php: Added other fields to the GP Notification Scheduler. Jan 10, 2025
@saifsultanc saifsultanc merged commit b119d2d into master Jan 10, 2025
3 checks passed
@saifsultanc saifsultanc deleted the saif/add/42388-add-other-fields-to-gpns branch January 10, 2025 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants