Push overhaul #4378

flovilmart · 2017-11-25T16:00:13Z

Motivation:

we send a decent volume of push notifications and we found that the current strategy for slicing / dicing push notification is not efficient
I noticed in the early commits of the dashboard, that pushStatus only have 3 possible states, pending, succeeded, failed. (thus we can don't need running, which caused issues before)
Marking push as succeeded, when the working items have been scheduled is reasonable, perhaps, we could introduce 2 modes into the PushQueue, direct sending (calling worker directly and queue), in the case of calling the worker directly we could 'wait' till all push are sent to mark complete.
I also noticed, many _PushStatus types are not implemented in the dashboard, including the experiments, campaigns etc... which could be easier to implement if we start sticking to the dashboard definitions instead.
This will is a breaking change for anyone using a custom push worker, or scheduler, as push won't be marked as scheduled etc... anymore.
A scheduled push is just a pending push with a push time. This will help us move into a more scalable / modular architecture.
Restores the translation key support as it was originally intended on parse-dashboard

This will require to bump to 3.0.

https://github.com/parse-community/parse-dashboard/commit/618ba05ad4c24414a3303a593bb712b7306d1e46\#diff-bd51845f946b451c46b25b33df0e85d4R196

codecov · 2017-11-25T18:22:46Z

Codecov Report

Merging #4378 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4378      +/-   ##
==========================================
+ Coverage   92.77%   92.78%   +<.01%     
==========================================
  Files         119      119              
  Lines        8448     8512      +64     
==========================================
+ Hits         7838     7898      +60     
- Misses        610      614       +4

Impacted Files	Coverage Δ
src/Push/PushQueue.js	`100% <100%> (ø)`	⬆️
src/Push/utils.js	`98.59% <100%> (+0.23%)`	⬆️
src/StatusHandler.js	`99.25% <100%> (-0.04%)`	⬇️
src/Controllers/PushController.js	`97.91% <100%> (+0.02%)`	⬆️
src/rest.js	`98.97% <100%> (+0.19%)`	⬆️
src/RestWrite.js	`93.5% <0%> (-0.19%)`	⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`96.76% <0%> (-0.11%)`	⬇️
src/Routers/ClassesRouter.js	`93.69% <0%> (+0.18%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7944e2b...56fd570. Read the comment docs.

… info

montymxb · 2017-11-25T23:48:51Z

spec/PushWorker.spec.js

@@ -157,6 +157,138 @@ describe('PushWorker', () => {
      expect(PushUtils.bodiesPerLocales({where: {}})).toEqual({default: {where: {}}});
      expect(PushUtils.groupByLocaleIdentifier([])).toEqual({default: []});
    });
+
+    it('should propely apply translations strings', () => {


typo on 'propely' 😛

meh.. look like I'm quite tired

montymxb · 2017-11-25T23:49:05Z

spec/PushWorker.spec.js

+      });
+    });
+
+    it('should propely apply translations objects', () => {


montymxb · 2017-11-25T23:49:18Z

spec/PushWorker.spec.js

+      });
+    });
+
+    it('should propely override alert-lang with translations', () => {


montymxb · 2017-11-25T23:49:28Z

spec/PushWorker.spec.js

+      });
+    });
+
+    it('should propely override alert-lang with translations strings', () => {


montymxb · 2017-11-25T23:55:35Z

src/Push/PushQueue.js

@@ -16,6 +18,7 @@ export class PushQueue {
  constructor(config: any = {}) {
    this.channel = config.channel || PushQueue.defaultPushChannel();
    this.batchSize = config.batchSize || DEFAULT_BATCH_SIZE;


Do we still need batchSize?

yeah we still need it so we have a query batch size and an enqueue batch size:

https://github.com/parse-community/parse-server/pull/4378/files#diff-6f234e6e995cfa72a42019190989aaa4R42

montymxb · 2017-11-26T00:04:58Z

spec/PushWorker.spec.js

@@ -276,7 +408,6 @@ describe('PushWorker', () => {
          'failedPerType.ios': { __op: 'Increment', amount: 1 },
          [`sentPerUTCOffset.${UTCOffset}`]: { __op: 'Increment', amount: 1 },
          [`failedPerUTCOffset.${UTCOffset}`]: { __op: 'Increment', amount: 1 },
-          count: { __op: 'Increment', amount: -1 }


If we're not using count on _PushStatus we should probably strip it from the schema as well, unless there's some particular need for it still.

montymxb · 2017-11-26T00:05:48Z

src/Push/PushQueue.js

@@ -2,9 +2,11 @@ import { ParseMessageQueue }      from '../ParseMessageQueue';
 import rest                       from '../rest';
 import { applyDeviceTokenExists } from './utils';
 import Parse from 'parse/node';
+import logger from '../logger';

 const PUSH_CHANNEL = 'parse-server-push';
 const DEFAULT_BATCH_SIZE = 100;


If batchSize is not needed we can just drop this too.

montymxb

Some typos and questions. But the tests look good 👍 .

flovilmart · 2017-11-27T14:53:04Z

src/Push/PushQueue.js

@@ -16,6 +18,7 @@ export class PushQueue {
  constructor(config: any = {}) {
    this.channel = config.channel || PushQueue.defaultPushChannel();
    this.batchSize = config.batchSize || DEFAULT_BATCH_SIZE;


yeah we still need it so we have a query batch size and an enqueue batch size:

https://github.com/parse-community/parse-server/pull/4378/files#diff-6f234e6e995cfa72a42019190989aaa4R42

flovilmart · 2017-11-27T14:53:14Z

src/Push/PushQueue.js

+      return rest.each(config, auth, '_Installation', where, options, (result) => {
+        total++;
+        currentBatch.push(result.objectId);
+        if (currentBatch.length == this.batchSize) {


it's used there.

flovilmart added discussion labels Nov 25, 2017

flovilmart requested review from montymxb and marvelm November 25, 2017 16:08

flovilmart force-pushed the push-overhaul-again branch from 17c8d4b to 94a060f Compare November 25, 2017 17:32

flovilmart added 5 commits November 25, 2017 12:43

Push overhaul

db091ba

Bumps to 3.0.0-alpha.1 as breaking lots of internals

cd28e25

Adds support for full translation payload, like original dashboard

164e833

https://github.com/parse-community/parse-dashboard/commit/618ba05ad4c24414a3303a593bb712b7306d1e46\#diff-bd51845f946b451c46b25b33df0e85d4R196

Adds tests for translation payload override

dd66e7a

Fixes issues with rest.each

a74e8bd

flovilmart force-pushed the push-overhaul-again branch from 94a060f to a74e8bd Compare November 25, 2017 17:43

parse-community deleted a comment from codecov bot Nov 25, 2017

flovilmart added 2 commits November 25, 2017 13:14

fixup! Push overhaul

4483900

Removes scheduled state from pushs, as redundant with pushTime info

1ad8ee0

fixup! Removes scheduled state from pushs, as redundant with pushTime…

56fd570

… info

flovilmart added this to the 3.0 milestone Nov 25, 2017

montymxb reviewed Nov 25, 2017

View reviewed changes

spec/PushWorker.spec.js

});

});

it('should propely apply translations objects', () => {

Copy link

Contributor

montymxb Nov 25, 2017

Choose a reason for hiding this comment

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

again here

montymxb reviewed Nov 25, 2017

View reviewed changes

spec/PushWorker.spec.js

});

});

it('should propely override alert-lang with translations', () => {

Copy link

Contributor

montymxb Nov 25, 2017

Choose a reason for hiding this comment

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

and here

montymxb reviewed Nov 25, 2017

View reviewed changes

spec/PushWorker.spec.js

});

});

it('should propely override alert-lang with translations strings', () => {

Copy link

Contributor

montymxb Nov 25, 2017

Choose a reason for hiding this comment

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

same

montymxb reviewed Nov 25, 2017

View reviewed changes

montymxb reviewed Nov 26, 2017

View reviewed changes

montymxb suggested changes Nov 26, 2017

View reviewed changes

flovilmart commented Nov 27, 2017

View reviewed changes

flovilmart closed this Aug 9, 2018

flovilmart deleted the push-overhaul-again branch September 25, 2018 01:27

flovilmart restored the push-overhaul-again branch September 25, 2018 01:27

TomWFox deleted the push-overhaul-again branch September 14, 2019 23:22

mtrezza removed the 🧬 enhancement label Jul 11, 2021

dplewis mentioned this pull request May 8, 2022

refactor: upgrade @graphql-tools/links from 8.2.11 to 8.2.13 #7989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push overhaul #4378

Push overhaul #4378

flovilmart commented Nov 25, 2017 •

edited

Loading

codecov bot commented Nov 25, 2017 •

edited

Loading

montymxb Nov 25, 2017

flovilmart Nov 25, 2017

montymxb Nov 25, 2017

montymxb Nov 25, 2017

montymxb Nov 25, 2017

montymxb Nov 25, 2017

flovilmart Nov 27, 2017

montymxb Nov 26, 2017

montymxb Nov 26, 2017

montymxb left a comment

flovilmart Nov 27, 2017

flovilmart Nov 27, 2017

montymxb Nov 28, 2017

Push overhaul #4378

Push overhaul #4378

Conversation

flovilmart commented Nov 25, 2017 • edited Loading

codecov bot commented Nov 25, 2017 • edited Loading

Codecov Report

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

montymxb 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

flovilmart commented Nov 25, 2017 •

edited

Loading

codecov bot commented Nov 25, 2017 •

edited

Loading