Conversation
* Helper method to check if only categories are applied. Usful in GCM land, where we use topics | ||
*/ | ||
@JsonIgnore | ||
public boolean isCategoryOnly() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps these methods should be moved to a util, next to TokenLoader?
Not sure if this is generally helpful on the Criteria
API class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@secondsun I thought about this, and will move this into a (GCM Topic) Util class.
|
||
private Set<String> extractTopicsFrom(final Criteria criteria, final String variantID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@secondsun this function is another candidate for the (GCM Topic) Util class.
I will update the PR...
6476891
to
b9ab1de
Compare
@secondsun ok, moved the TokenLoader around and was adding a util, instead of blowing up the public Criteria API (used on the java sender too) |
Some testing info, @secondsun @matskiv I was using an older app (e.g. old hello world, which uses GCM register to get a legacy token)
It first sends to the topic, but also looks for legacy tokens, as fallback. Message is delivered (since my device is in the 'soccer' category) ✅
It first sends to the 'global; topic, but also looks for legacy tokens, as fallback. Message is delivered. ✅
First sends to the given topic (or category), afterwards looks for the tokens - but there is no token, as no device is registered for this category. Ergo, no message is delivered ✅ |
LGTM +1 |
feel free to merge! On Tuesday, 3 May 2016, Hoyt Summers Pittman notifications@github.com
Sent from Gmail Mobile |
landed - will send different PR for |
Done for AGPUSH-1454
@secondsun @matskiv this is the PR for the 1.1.x series. Once this is merged, I will get #708 updated and land it on master