[WIP]Refactor to Traits and add some little more Parameters #58

LKaemmerling · 2018-03-14T06:11:48Z

This PR aims to provide a cleaner Codebase and make it possible to natively set more Parameters on the Notification Creation.

Since this PR isn't ready yet, it should be a little bit a discussion between all users. Feel free to checkout the branch and make Changes too or just test it :)

Actually, i think this could be a complete rewrite known as 2.0 or what do you think?
CC @timacdonald @Lloople

// StyleCi will be fixed after this PR is feature complete

timacdonald · 2018-03-14T06:18:04Z

You know I was thinking something similar to this when I was adding my PR's. I was thinking you could ditch all the protected properties and just having a $payload variable that was built. The idea being that toArray() wouldn't need to do anything but return the $payload variable.

public function icon($value)
{
    $this->icon = $value;

    return $this;
}

becomes

public function icon($value)
{
    return $this->setParameter('chrome_web_icon', $value)
                ->setParameter('chrome_icon', $value)
                ->setParameter('adm_small_icon', $value)
                ->setParameter('small_icon', $value);
}

Because now that you've added the parameter helper, its like there is these "special" parameters that have instance properties - and everything else would just be set via setParameter which feels weird to me. I think standardising the way the payload is built up would feel much nicer IMO

Cause at the end of the day - they are all just parameters - even the $data could just be on the payload..

public function setData($key, $value)
{
    return $this->setParameter("data.{$key}", $value);
}

then in the setParameter method would just use Illuminate\Support\Arr to set values, allowing for "dot notation"

Keep Backward Compatibility

timacdonald · 2018-03-14T06:42:29Z

haha, judging by the PR's coming in - I spoke too soon haha - your all over it 😅

LKaemmerling · 2018-03-14T06:43:27Z

Yeah, your right :)

I've refactored it, so there aren't any unused protected variables there anymore. I'd leave the "setData" Method on the Class, since it could be usefull for some people to set the data and because auf the Backward Compability i'd leave the body, subject & url method

Edit: Ha what a good idea with the data :D, did it :D

…tation on setParameter

Lloople

Looks pretty good! This code organization is very beatufiul 🙂

timacdonald · 2018-03-14T07:20:49Z

I'll pull it down and check it out - looks nice :)

timacdonald

I hope you don't mind the feedback. I know I'm not a maintainer - so it's probably not my place - but thought I'd throw my thoughts in the mix.

timacdonald · 2018-03-14T07:25:22Z

src/OneSignalMessage.php

-
-    /** @var string */
-    protected $icon;
+    use OneSignalHelpers;

    /** @var array */
    protected $data = [];


can ditch this var now as we're going straight onto the parameter in the setData method

timacdonald · 2018-03-14T07:28:07Z

src/OneSignalMessage.php

-    public function icon($value)
-    {
-        $this->icon = $value;
+        if (is_array($value)) {


Could ditch the else here if you wanted to with an early return

public function body($value) { if (is_array($value)) { return $this->setParameter('contents', $value); } return $this->setParameter('contents', ['en' => $value]); }

timacdonald · 2018-03-14T07:35:42Z

src/OneSignalMessage.php

+            $this->setParameter('headings', $value);
+        } else {
+            $this->setParameter('headings', ['en' => $value]);
+        }


Same here as with body - however if going with the array check version, you might want to introduce a parseLocale() method to remove the duplicate code...

protected function parseLocale($value) { return is_array($value) ? $value : ['en' => $value]; } // then subject / body etc with localisation could just be public function subject() { $this->setParameter('headings', $this->parseLocale($value)); }

Yeah thats a better solution!

timacdonald · 2018-03-14T07:39:57Z

src/Traits/Categories/AppearanceHelpers.php

+     *
+     * @return $this
+     */
+    public function setIosSound(string $soundUrl)


I kinda feel like "sound" is not "appearance" ...maybe

I've used the "categories" from the OneSignal Documentation for the "grouping" of the methods:
https://documentation.onesignal.com/reference
And there are the sound Settings under "Appearance" :)

oh - that makes way more sense haha - sorry should have checked that :) I like that 👍

timacdonald · 2018-03-14T07:40:43Z

src/Traits/Categories/AppearanceHelpers.php

+     */
+    public function setWindowsSound(string $soundUrl)
+    {
+        return $this->setParameter('wp_sound', $soundUrl)->setParameter('wp_wns_sound', $soundUrl);


->setParameter('wp_wns_sound', $soundUrl); on a new line here for consistency.

timacdonald · 2018-03-14T07:51:04Z

src/Traits/Categories/ButtonHelpers.php

+     *
+     * @return $this
+     */
+    public function button(OneSignalButton $button)


do you think maybe this should be prefixed. For example - take the notifications conversation we had:

incrementBadgeCount() sound like it will do it everywhere.

incrementIosBadgeCount() sound like it will do it on iOS.

webButtons sounds like it is for the web (which is it)

buttons sounds like it will do it everywhere - web + native

Yeah i agree with you, but i think this is something that should clarify first, since they named it "web buttons" and "buttons"
https://documentation.onesignal.com/reference#section-action-buttons

uh! yea - thats tough because I think matching their docs is good!

timacdonald · 2018-03-14T07:52:24Z

src/Traits/Categories/DeliveryHelpers.php

+     *
+     * @return $this
+     */
+    public function setTTL(int $ttl)


As we are going with Ios of iOS, might want to do Ttl. I hate this capitalisation stuff with acronyms like that i.e Html etc. always feels weird.

Yeah you are right :)

timacdonald · 2018-03-14T07:52:46Z

src/Traits/Categories/GroupingHelpers.php

+{
+    /**
+     * Set the Android Grouping Parameters.
+     * @param string $group


new line between description and @param

timacdonald · 2018-03-14T07:53:04Z

src/Traits/Categories/GroupingHelpers.php

+
+    /**
+     * Set the Amazon (FireOS) Grouping Parameters.
+     * @param string $group


new line between description and @param

timacdonald · 2018-03-14T07:54:15Z

src/Traits/OneSignalHelpers.php

@@ -0,0 +1,14 @@
+<?php


I think you are probably better off just useing these in the OneSignalMessage class rather than using a trait that is only there to use other traits IMO

timacdonald · 2018-03-14T08:02:00Z

Please note - I don't maintain anything like this - so I feel bad for leaving so much feedback as just a random contributor - but I want to say this is awesome and will really clean things up

timacdonald · 2018-03-14T08:12:05Z

As all the introduced methods are actionThing i.e. setImage incrementIosBadgeCount perhaps we should introduce setX methods for the following - just to standardise the API:

icon()
webButton()
webButtons()
button()
buttons()
body()
subject()

LKaemmerling · 2018-03-14T09:51:27Z

@timacdonald the problem with this methods is the backward compability... i don't want to break alle applications. But you are right,
a better solution were to deprecate the old method and make them an alias for the new.

LKaemmerling · 2018-03-14T10:01:30Z

@timacdonald should be done now :)

Thank you really for all your help and ideas on this!

timacdonald · 2018-03-14T21:09:45Z

No worries - thanks for keeping this package alive and kicking. I’ll take another looksy with fresh eyes this morning.

Yea I was thinking alias the old methods to the new ones to keep that backwards compat is a good idea.

timacdonald

This is totally fine - but I made a couple of comments. They are probably just personal preference things - so might not really benefit the code at all. Looking good!

timacdonald · 2018-03-14T22:43:05Z

src/OneSignalMessage.php

     *
+     * @deprecated use setBody instead
+     *             
     * @return $this
     */
    public function body($value)


I wonder if it would be good to move these deprecated methods to Traits\Deprecated to clean things up?

timacdonald · 2018-03-14T22:49:42Z

src/Traits/Categories/ButtonHelpers.php

+     */
+    public function setWebButtons(array $buttons)
+    {
+        return $this->setParameter('web_buttons', collect($buttons)->map(function ($button) {


This is probably fine - but you could just make use of the existing method here:

collect($buttons)->each(function ($button) { $this->setWebButton($button); });

That way if the key every changes you only have one place to manage it - but again, probably not needed - just an idea :)

Could also apply to setButtons?

timacdonald · 2018-03-14T22:57:58Z

src/OneSignalMessage.php


-    /** @var array */
-    protected $buttons = [];
+    use AppearanceHelpers, AttachmentHelpers, ButtonHelpers, DeliveryHelpers, GroupingHelpers;


You could use relative trait names here - then you could remove all of the top use imports.

i.e.

class OneSignalMessage { use Traits\Categories\ButtonHelpers, Traits\Categories\GroupingHelpers, Traits\Categories\DeliveryHelpers, Traits\Categories\AppearanceHelpers, Traits\Categories\AttachmentHelpers;

But that is just a style thing - and not really needed

timacdonald · 2018-03-14T23:01:54Z

Thats probably the extent of my feedback - it's looking good and tests are passing!

Edit: Maybe one last thing to consider is if there are any other methods that you previously could chain to ensure you still can.

Add Test for Chaning Buttons (close #60)

LKaemmerling · 2018-03-15T08:11:49Z

@timacdonald Thank you for all your comments & feedback! I'v edit the points. The next days i'll add tests for all of the new methods and i'll try to refactor the tests as well

timacdonald · 2018-03-15T08:13:26Z

No probs at all!

Fixed Maximum function nesting level of '256' reached, aborting!

LKaemmerling · 2018-03-23T08:54:15Z

Sorry guys, that i currently do nothing here. This week was a little bit crazy and i haven't time for it.

A defensive change in message class adding support for silent push notifications (in the traits branch now).

LKaemmerling · 2018-05-22T08:19:04Z

@kjostling Can you test this branch? When you can't find an error too, we can release it as a rc :)

LKaemmerling · 2018-06-10T19:43:08Z

@timacdonald @kjostling have you tested this? Can we release it :)?

timacdonald · 2018-06-13T01:19:25Z

Hey,

I haven't had a chance to test this myself - but I trust you 👍

* Add method to set the template_id of the message * Fix typo on method for setting template * Remove contents when using a template Remove any contents set by the constructor when using the `setTemplate()` method.

kjostling · 2018-08-15T14:44:20Z

Sorry for late reply, im already using this in a production env. Believe its tested from my point of view.

LKaemmerling · 2018-08-16T07:56:19Z

Okay good :) Then i will fix the styleci errors and then i will prepare the release

…uded) (#72) * implemented sending of notifications based on segments (included and excluded) * styleci linting * Apply fixes from StyleCI

* ability of user to send notification , filtering for multiple tags * added linting * Apply fixes from StyleCI

…r-to-traits

LKaemmerling added 2 commits March 14, 2018 07:08

Refactor to Traits and add some little more Parameters

715521e

Add Grouping Helpers

3c7ae6a

LKaemmerling added 2 commits March 14, 2018 07:29

Add Button Helpers

7d42ea1

Removed all unused protected variables

e00b7cc

Keep Backward Compatibility

LKaemmerling and others added 2 commits March 14, 2018 07:46

Make the setData Method call setParameter directly & allow the dot no…

95fe263

…tation on setParameter

Apply fixes from StyleCI (#59)

b926b13

Lloople approved these changes Mar 14, 2018

View reviewed changes

timacdonald reviewed Mar 14, 2018

View reviewed changes

LKaemmerling added 2 commits March 14, 2018 09:04

Working on Code with suggestions from timacdonald

7101895

Working on Code with suggestions from timacdonald #2

5095faf

Add Deprecated DocTypes & remove Trait the contains only other Traits

3be802c

timacdonald reviewed Mar 14, 2018

View reviewed changes

Add Deprecated Trait that contains all deprecated methods

ad05ee0

Add Test for Chaning Buttons (close #60)

i really love travis :D

d14ac9a

Fixed Maximum function nesting level of '256' reached, aborting!

Add support for silent messages (#67)

52bfc9e

A defensive change in message class adding support for silent push notifications (in the traits branch now).

Add method to set the template_id of the message (#77)

b1e9d1b

* Add method to set the template_id of the message * Fix typo on method for setting template * Remove contents when using a template Remove any contents set by the constructor when using the `setTemplate()` method.

LKaemmerling and others added 9 commits August 16, 2018 09:58

Fix Style CI

a14695b

Fix Style CI Issues

860ce55

Fix Style CI Issues

64bb529

implemented sending of notifications based on segments (included/excl…

32521dd

…uded) (#72) * implemented sending of notifications based on segments (included and excluded) * styleci linting * Apply fixes from StyleCI

ability of user to send notification , filtering for multiple tags (#73)

83c1c00

* ability of user to send notification , filtering for multiple tags * added linting * Apply fixes from StyleCI

Pass Notification Object to Route

7d7edcb

Merge remote-tracking branch 'origin/refactor-to-traits' into refacto…

785705b

…r-to-traits

Fix Style CI Issue

5431c23

Fix Style CI Issue

d5a94ab

LKaemmerling merged commit 84d6e05 into master Aug 16, 2018

[WIP]Refactor to Traits and add some little more Parameters #58

[WIP]Refactor to Traits and add some little more Parameters #58

Conversation

LKaemmerling commented Mar 14, 2018 • edited Loading

timacdonald commented Mar 14, 2018 • edited Loading

timacdonald commented Mar 14, 2018

LKaemmerling commented Mar 14, 2018 • edited Loading

Lloople left a comment

Choose a reason for hiding this comment

timacdonald commented Mar 14, 2018

timacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timacdonald commented Mar 14, 2018

timacdonald commented Mar 14, 2018

LKaemmerling commented Mar 14, 2018

LKaemmerling commented Mar 14, 2018

timacdonald commented Mar 14, 2018

timacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timacdonald commented Mar 14, 2018 • edited Loading

LKaemmerling commented Mar 15, 2018 • edited Loading

timacdonald commented Mar 15, 2018

LKaemmerling commented Mar 23, 2018

LKaemmerling commented May 22, 2018

LKaemmerling commented Jun 10, 2018

timacdonald commented Jun 13, 2018

kjostling commented Aug 15, 2018

LKaemmerling commented Aug 16, 2018

LKaemmerling commented Mar 14, 2018 •

edited

Loading

timacdonald commented Mar 14, 2018 •

edited

Loading

LKaemmerling commented Mar 14, 2018 •

edited

Loading

timacdonald commented Mar 14, 2018 •

edited

Loading

LKaemmerling commented Mar 15, 2018 •

edited

Loading