-
-
Notifications
You must be signed in to change notification settings - Fork 623
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
Add Edge support to webpush #631
Conversation
Show another, easier approach to extract the browsername and browser version
Codecov Report
@@ Coverage Diff @@
## master #631 +/- ##
==========================================
- Coverage 68.24% 68.21% -0.03%
==========================================
Files 24 24
Lines 1099 1098 -1
Branches 173 173
==========================================
- Hits 750 749 -1
Misses 312 312
Partials 37 37
Continue to review full report at Codecov.
|
@simonkern I don't know if this statement is correct, but the edge url seems to change all the time? Is it possible to make this an EDGE setting? |
@@ -41,6 +41,7 @@ | |||
"CHROME": PUSH_NOTIFICATIONS_SETTINGS["FCM_POST_URL"], | |||
"OPERA": PUSH_NOTIFICATIONS_SETTINGS["FCM_POST_URL"], | |||
"FIREFOX": "https://updates.push.services.mozilla.com/wpush/v2", | |||
"EDGE": "https://wns2-par02p.notify.windows.com/w", |
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.
I'm a little confused where this url comes from? Is it not cloud.notify.windows.com?
@@ -240,7 +241,7 @@ class WebPushDevice(Device): | |||
browser = models.CharField( | |||
verbose_name=_("Browser"), max_length=10, | |||
choices=BROWSER_TYPES, default=BROWSER_TYPES[0][0], | |||
help_text=_("Currently only support to Chrome, Firefox and Opera browsers") | |||
help_text=_("Currently only support to Chrome, Firefox, Edge and Opera browsers") |
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.
Please create a migration!
I was not able to produce an URL with another domain so far. However, I was also unable to find proper documentation on this matter. But if this URL would change all the time, it would be necessary to adapt the model to store the baseURL as well, imho. What do you think and suggest? |
I'm not sure what you mean by adapt the model to store the URL. I was suggesting in settings.py, instead of hard coding the url, allow for user customization like the chrome url. Additionally, make sure to create a migration! |
Doesn't WP_POST_URL already solve this? |
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.
ah gotcha sorry 😅 LGTM then
AHH but wait migration file! |
Oh sorry, I forgot to include the migration, it's now included. |
Thanks again! |
* webpush add Edge * webpush - add edge endpoint * change edge endpoint * add vscode files to gitignore * update README Show another, easier approach to extract the browsername and browser version * formatting * migration for webpush with edge
This adds Edge support to webpush and showcases an easier way to extract browsername and version for Chrome, Edge, and Opera in the README.
Any feedback would be greatly appreciated.