[Push Notification] Added support for no channels #150

fabiomassimo · 2014-11-09T02:59:44Z

Push Notification:

If there are no channels (default no channel), the push notification
is sent to all target devices. (issue #140)
Improved support for targeting by platform: a device type is specified as where
clause
Improved support for advanced targeting: if a channel is specified it's also possible to use where clause in it.

## Push Notification: - If there are no channels (default no channel), the push notification is sent to all target devices. - Improved support for targeting by platform: a device type is specified as [where clause](https://www.parse.com/docs/push_guide#options-platform/REST)

- Updated test pattern for new “no channel” support compatibility. - Fixed bug when deviceType and where clause are specified at same time.

ck3g · 2014-11-09T06:46:13Z

@fabiomassimo Could you at least use two spaces instead of tabs(or 4 spaces), please? Current indentation are broken.
https://github.com/styleguide/ruby
https://github.com/bbatsov/ruby-style-guide#spaces-indentation

Second. You might broke backward compatibility by replacing default channel from "", to nil. Some people might use it to broadcast messages.

Why are you double checking for !channel?

if !channel
  @where = {} if !channel
else

- Reverted tabs to soft-spaces as required from [ruby style guide](https://github.com/styleguide/ruby). - Reverted channel default value to “” - Removed superfluous code.

fabiomassimo · 2014-11-09T09:10:05Z

https://github.com/styleguide/ruby
https://github.com/bbatsov/ruby-style-guide#spaces-indentation

Thanks for the great links. This is my first ruby experience. I will definitely look into it.

Second. You might broke backward compatibility by replacing default channel from "", to nil. Some people might use it to broadcast messages.

By using channel=nil in method signature, it only specifies a default value if none has been set, right?
If the user wants to broadcast a message to a channel, when the second parameters is specified it should work. I've reverted it to an empty string anyway.

Why are you double checking for !channel?

Thanks that was a superfluous code.

ck3g · 2014-11-09T09:59:07Z

Someone, who is already using a gem, may use push = Parse::Push.new(data) to send to channel ""

fabiomassimo · 2014-11-09T10:19:16Z

I don't think an empty string could be a valid channel.

ck3g · 2014-11-09T18:10:36Z

Empty string is a Broadcast channel

ck3g · 2014-11-09T18:13:12Z

test/test_push.rb

@@ -1,45 +1,47 @@
 require 'helper'

 class TestPush < ParseTestCase
-
+    


Could you get rid of trailing whitespaces, please?

Removed trailing whitespaces.
Thanks.

Removed trailing superfluous spaces

ck3g · 2014-11-09T19:21:33Z

lib/parse/push.rb

@@ -15,7 +15,13 @@ class Push

    def initialize(data, channel = "")
      @data = data
-      @channel = channel
+
+      if channel.empty?


If someone will set channel as nil explicitly you would get

NoMethodError: undefined method `empty?' for nil:NilClass`

Improved code consistency if `initialize` a push notification with `nil` channel.

koenpunt · 2014-11-09T22:47:03Z

lib/parse/push.rb

@@ -15,7 +15,13 @@ class Push

    def initialize(data, channel = "")
      @data = data
-      @channel = channel
+
+      if !channel || channel.empty?


If you change the default for channel from "" to nil, you can simplify this to if channel.nil?

What if an user initiate a new Push object with: Push.new(data, "")?
The channel parameter is not nil but has the same semantic value of nil.
I used nil as default parameter value in my first commit but than @ck3g pointed out backward compatibility issues like the one I just show you.
What do you think?

Ok, for bc. But if you take backwards compatibility serious, you should not alter the tests, but add new ones.

I think the tests were inconsistent with Parse REST documentation. That's why I decided to change them.
Previously a simple request with device type set made the test failed because it checked no where clause was created but if you want to specify a device type you have to add it as where clause.
Same goes with nil channel.
If you want to send a broadcast message, you have to add a where clause, hence a simple request could have a where clause and a channel specification.

Fair point. And even it's not my responsibility to review your PR, I suggest submitting those changes as separate PR's.

Submitting test pattern changes as different PR will make this PR never pass the Travis check. Or should I ignore it and make a new PR for test pattern changes?

I run into that issue from time to time as well.
What if you rebase and merge some of your commits so that you only have 2 left; one with the test pattern changes and one with the other changes. Then you can force push the first commit, let Travis do its thing, and after that push the second commit.
This way it's easier to see what happened and what's the results on Travis per commit.

Changed order check of included parameters in body request to send to Parse.

xavdid · 2015-06-03T20:13:46Z

@fabiomassimo hey, we can't auto merge this anymore because of other changes. If you'd like to get this in, could you merge master back into your branch and push that?

…lizer

rhymes · 2015-06-03T23:19:35Z

@Xavdidtheshadow not sure what problem this PR solves.

koenpunt · 2015-06-04T07:13:08Z

@rhymes see #140

rhymes · 2015-06-04T07:44:10Z

@koenpunt thanks!

fabiomassimo · 2015-06-06T21:43:20Z

@Xavdidtheshadow thanks for taking care of this pr. Is there anything else I can do to help you out?

rhymes · 2015-06-07T11:46:58Z

@fabiomassimo thanks for the fix! You just need to update your branch with the current master and fix the conflicts so that your PR can be merged.

See https://help.github.com/articles/syncing-a-fork/

…into no_channel_support

fabiomassimo · 2015-06-07T14:25:51Z

I just merged the latest changes from current master branch but I didn't come across any merge conflict. Am I missing something?

xavdid · 2015-06-07T15:22:17Z

Did you checkout and git pull each branch before merging?

~DB

On Sun, Jun 7, 2015 at 9:25 AM, Fabio [email protected] wrote:

I just merged the latest changes from current master branch but I didn't come across any merge conflict. Am I missing something?

Reply to this email directly or view it on GitHub:
#150 (comment)

xavdid · 2015-06-07T15:45:12Z

Sorry, wasn't clear. I meant pulled current master and merged that. Sometimes people forget to pull and then merge without conflict. But not that though, huh?

I'm on mobile now, but I can actually dig into what the conflicts are later if you haven't found them by then.

~DB

On Sun, Jun 7, 2015 at 10:37 AM, Fabio [email protected] wrote:

I merged only from current master branch (main repo).

Why should I merge all branches?

Reply to this email directly or view it on GitHub:
#150 (comment)

fabiomassimo · 2015-06-07T20:01:28Z

@Xavdidtheshadow thanks! I didn't encounter any conflict indeed. Let me know if I'm missing something. It seems good for me..

xavdid · 2015-06-08T04:47:09Z

@fabiomassimo nice, the conflict seems to have resolved itself. Merged!

[Push Notification] Added support for no channels

rhymes · 2015-06-08T08:54:26Z

@Xavdidtheshadow just a thought, with my push and this we should probably start improving the documentation because they are both changes that modify the behavior

xavdid · 2015-06-08T15:58:11Z

@rhymes totally agree. Do you mind adding relevant documentation as part of #167?

rhymes · 2015-06-08T18:02:43Z

@Xavdidtheshadow done

fabiomassimo added 2 commits November 9, 2014 03:55

[Push Notificaiton] Updated test patterns

a2aeeee

- Updated test pattern for new “no channel” support compatibility. - Fixed bug when deviceType and where clause are specified at same time.

fabiomassimo mentioned this pull request Nov 9, 2014

Push not working #140

Closed

Code style and backward compatibility

9f059dc

- Reverted tabs to soft-spaces as required from [ruby style guide](https://github.com/styleguide/ruby). - Reverted channel default value to “” - Removed superfluous code.

ck3g reviewed Nov 9, 2014
View reviewed changes

Removed superfluous trailing spaces

26499a6

Removed trailing superfluous spaces

ck3g reviewed Nov 9, 2014
View reviewed changes

Added support for channel set to nil

15d5c62

Improved code consistency if `initialize` a push notification with `nil` channel.

koenpunt reviewed Nov 9, 2014
View reviewed changes

Improved code readability and consistency

962473e

Changed order check of included parameters in body request to send to Parse.

xavdid added in_progress feature labels Jun 3, 2015

Fabio Milano added 4 commits June 3, 2015 22:34

Merge branch 'master' of https://github.com/adelevie/parse-ruby-client

40bdeee

Merge branch 'master' into no_channel_support

36cc2e6

Added support for specified Parse client instantiation in push initia…

ab42ba5

…lizer

Updated push test accordingly to Parse push documentation

635345d

Merge branch 'master' of https://github.com/adelevie/parse-ruby-client …

2e25134

…into no_channel_support

xavdid added a commit that referenced this pull request Jun 8, 2015

Merge pull request #150 from fabiomassimo/no_channel_support

6898d6b

[Push Notification] Added support for no channels

xavdid merged commit 6898d6b into adelevie:master Jun 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Push Notification] Added support for no channels #150

[Push Notification] Added support for no channels #150

fabiomassimo commented Nov 9, 2014

ck3g commented Nov 9, 2014

fabiomassimo commented Nov 9, 2014

ck3g commented Nov 9, 2014

fabiomassimo commented Nov 9, 2014

ck3g commented Nov 9, 2014

ck3g Nov 9, 2014

fabiomassimo Nov 9, 2014

ck3g Nov 9, 2014

koenpunt Nov 9, 2014

fabiomassimo Nov 10, 2014

koenpunt Nov 10, 2014

fabiomassimo Nov 10, 2014

koenpunt Nov 10, 2014

fabiomassimo Nov 10, 2014

koenpunt Nov 10, 2014

xavdid commented Jun 3, 2015

rhymes commented Jun 3, 2015

koenpunt commented Jun 4, 2015

rhymes commented Jun 4, 2015

fabiomassimo commented Jun 6, 2015

rhymes commented Jun 7, 2015

fabiomassimo commented Jun 7, 2015

xavdid commented Jun 7, 2015

I just merged the latest changes from current master branch but I didn't come across any merge conflict. Am I missing something?

xavdid commented Jun 7, 2015

Why should I merge all branches?

fabiomassimo commented Jun 7, 2015

xavdid commented Jun 8, 2015

rhymes commented Jun 8, 2015

xavdid commented Jun 8, 2015

rhymes commented Jun 8, 2015

		@@ -1,45 +1,47 @@
		require 'helper'

		class TestPush < ParseTestCase

[Push Notification] Added support for no channels #150

[Push Notification] Added support for no channels #150

Conversation

fabiomassimo commented Nov 9, 2014