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

Accept grouping options for notification #75

Conversation

Kaname87
Copy link

@Kaname87 Kaname87 commented Feb 16, 2019

Status

READY

Description

Partially resolves 74

In this PR just only making component and action acceptable among the notification options.
fingerprint is also grouping options but that is not a part of request but a part of error so not included in this change.

Related PRs

na

Todos

  • Tests
  • Documentation
  • Changelog Entry (unreleased)

Steps to Test or Reproduce

> git pull --prune
> git checkout <branch>
> vendor/bin/phpunit

@sixlive
Copy link
Collaborator

sixlive commented Feb 24, 2019

Rather than do some review here I opened a PR with come updates to your repo/branch.

@Kaname87
Copy link
Author

@sixlive sorry i merged your pr but does your 'wip" commit comment mean you were still about to change or refactor something ? if so i can revert the change and can remove merged commit log also if needed.

@sixlive
Copy link
Collaborator

sixlive commented Feb 25, 2019

It was ready for you to merge, I forgot to squash and rewrite before opening. I see one more change that I need to make though. I just want to verify in the HB UI and we should be good to go. I’ll update tonight.

public static function get($array, $key, $default = null)
{
if (! static::accessible($array)) {
return value($default);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return value($default);
return $default;

@sixlive
Copy link
Collaborator

sixlive commented Feb 26, 2019

I did an e2e test and it looks like its reporting as expected!

screen shot 2019-02-26 at 10 25 32 am

@sixlive
Copy link
Collaborator

sixlive commented Jul 15, 2019

Closing in favor of #85

@sixlive sixlive closed this Jul 15, 2019
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.

2 participants