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

Push notifications #429

Open
wants to merge 156 commits into
base: develop
Choose a base branch
from
Open

Conversation

mpw-wwu
Copy link
Collaborator

@mpw-wwu mpw-wwu commented Oct 21, 2019

Hello Co-Developers,

this is the push-code that I've written so far. It is basically in working state. But has some issues that need to be addressed.

This pull request is not meant to be merged. It's just a basis for discussion of further steps.

  • payload needs to include the link to the run
  • Allow configuration of push message
  • Automatic VAPID key generation (easier setup of new instances)
    • include PHP library for VAPID key generation
    • generate VAPID keys if not yet present
    • insert them into the JS-Code automatically
  • Properly ask for push subscription in frontend code
  • Prevent multiple registration attempts
  • Strategic decision: seperation of push notifications and email (probably some kind of fallback will be needed)
  • Discuss scope of service worker: One sw for both admin area and runs or seperate ones?
  • If session is deleted, all push subscriptions should be deleted from survey_push_subscriptions

Best,
Matthias

cyriltata and others added 30 commits July 12, 2018 16:35
Adjusted setup process to Debian 9.
Setting paragonie/halite dependency to '*' and leave it up to composer
…hpspreadsheet

Replaced deprecated PHPExcel by PhpSpreadsheet
…g-path

Cronjobs typically have a very limited PATH environement.
…TP-connections

Allow sending emails without authentification.
…include-028

Update sql/schema.sql and include patches/028_add_user_attributes.sql
dependencies and configuration on latest experiences.
…lts-table-by-id

Order columns in result table by id
…t-import

Fix Google Spreadsheet import on PHP7.2.
…-module-name

Extend “Log of user activity” by column “Module Description”.
showif fixes, allow more choice values, docs
…ckbox_to_select_all

Add checkbox to select (or unselect) all user sessions at once.
# Conflicts:
#	webroot/assets/build/js/formr-admin.min.js
"background_color": "#000000",
"display": "browser",
"Scope": "PsyTD/formr-entwicklung",
"start_url": "https://www.uni-muenster.de/PsyTD/formr-entwicklung",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to check if we can insert this dynamically.

webroot/sw.js Outdated
});

if (client !== undefined) {
client.navigate('https://www.uni-muenster.de/PsyTD/formr-entwicklung/NeuesQueuingSystemTest');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The link to the run needs to be inserted here. Should be part of the push notification.

@cyriltata
Copy link
Collaborator

@mpw-wwu Please could you create a new feature branch too called feature/push-notifications under rubenarslan/formr.org ? and let's merge this pull request there?

@mpw-wwu
Copy link
Collaborator Author

mpw-wwu commented Oct 22, 2019

Sure I can do that. Having a pull request allows us to view the changes easily on here and do reviews on the code as Github provides a nice web ui for that. I'll do some more changes today and push it into the requested feature branch.

@mpw-wwu
Copy link
Collaborator Author

mpw-wwu commented Oct 22, 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.

4 participants