Skip to content

Add data to notification#90

Merged
shadyvb merged 3 commits into
dynamic-action-datafrom
dynamic-notification-data
Jul 9, 2019
Merged

Add data to notification#90
shadyvb merged 3 commits into
dynamic-action-datafrom
dynamic-notification-data

Conversation

@mattheu
Copy link
Copy Markdown
Member

@mattheu mattheu commented Jul 8, 2019

Here is some POC code to add data to the notification itself (instead of or in addition to action data...)

Mostly just getting my head around the internals of this plugin so needs a bit more work - but wanted to share.

@mattheu
Copy link
Copy Markdown
Member Author

mattheu commented Jul 8, 2019

Oh another thing I added here is a timestamp, which I think would be useful to include in this.

Copy link
Copy Markdown
Contributor

@roborourke roborourke left a comment

Choose a reason for hiding this comment

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

I'm curious whether it's worth going straight for #89 at this juncture. These are useful additions though. I'm thinking what I mention in my last comment on #88 would be handy too

Comment thread inc/class-workflow.php Outdated
Comment thread lib/destinations/dashboard.php Outdated
Copy link
Copy Markdown
Contributor

@shadyvb shadyvb left a comment

Choose a reason for hiding this comment

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

Could use a new type property while we’re here, otherwise no blockers, will need more work on sanitisation once merged.

Comment thread inc/class-workflow.php
$data_sanitization_fn = apply_filters( 'hm.workflows.action.data.sanitizer', 'sanitize_text_field' );

if ( ! is_callable( $data_sanitization_fn ) ) {
if ( ! $data_sanitization_fn || ! is_callable( $data_sanitization_fn ) ) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can probably make use of the enhancements Rob suggested, where a more thorough function is checking object items based on the value type. It needs some work on the original ticket anyway, and probably then some.

@shadyvb shadyvb mentioned this pull request Jul 9, 2019
2 tasks
@shadyvb
Copy link
Copy Markdown
Contributor

shadyvb commented Jul 9, 2019

Continuing work on this as part of #88, will tackle all the feedback here.

@shadyvb shadyvb merged commit b8b1002 into dynamic-action-data Jul 9, 2019
@shadyvb shadyvb deleted the dynamic-notification-data branch July 9, 2019 10:16
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.

3 participants