Front/webpush#2663
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
b74d5a6 to
4456cb0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
f029393 to
3b9f200
Compare
|
I've rebased the PR on the master branch, and updated following some changes we did: apptype (array) => appTypes (String) |
3b9f200 to
ec63ade
Compare
|
I really love this PR and I wanted WebPush support in Nextcloud for some time already, so I am super sorry for all this! But another thing I noticed was, that you don’t seem to handle delete notifications at all? I am not even sure, how this could be properly handled. You could call I assume, integrating UnifiedPush for the mobile apps will be a lot more straight forward as most logic should already be in place there. It’s just a shame, that you have to do all this manual legwork for browsers. 😅 |
|
I suppose this is a typo and you're not sorry for this ? BTW, your help is very welcome, thanks. I will show a generic notification if we don't have the right content. You can also suggest code directly or open PR on my repo if you want :) Regarding UnifiedPush: the implementation is already done, I've a few things to do following changes on the backend Pr :D |
|
I just didn't want to come across as rude and complaining about a lot, especially since I am not affiliated with Nextcloud. :) And nice, I will see what my time allows me to do on contributing. :) I’m looking forward to the UnifiedPush implementation! |
|
No problem, code review is done for that ! At the end we have a better solution |
5e1deb0 to
cb3b999
Compare
Antreesy
left a comment
There was a problem hiding this comment.
From my POV frontend changes look fine. We tried it on a test instance, looks like working =)
regarding putting i file in /public and then having it on a root - if it's not an issue to have it on master branch, I can address it in a follow-up, it's not really related to webpush to block this PR from merging
nickvergessen
left a comment
There was a problem hiding this comment.
Otherwise good to go from API side
| } | ||
|
|
||
| /** | ||
| * @return Int tokenId from IToken or ISession, negative in case of Session id, positive otherwise |
There was a problem hiding this comment.
I'm a bit scare about this part.
I think I'd prefer having a second db field instead
| protected function validateToken(int $tokenId, int $maxAge): TokenValidation { | ||
| // This is a web session token | ||
| if ($tokenId < 0) { | ||
| return TokenValidation::VALID; |
There was a problem hiding this comment.
Why is it okay for a web session to be always valid?
There was a problem hiding this comment.
They are Session and not Token, and $this->tokenProvider->getTokenById($tokenId) doesn't work for the session.
The idea is if the web push registration is removed from the browser, or expire because the user doesn't use their browser for some time, the push server will respond with 404 or 410, and the token will be wiped at this moment
Antreesy
left a comment
There was a problem hiding this comment.
LGTM, pls maybe rebase and squash it up to 1-3 commits. Also double check DCO and other CI failures - NPM build we can trigger later separately
1239274 to
089b7cc
Compare
Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com> Signed-off-by: S1m <31284753+p1gp1g@users.noreply.github.com>
…eWorker Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Co-authored-by: Maksim Sukharev <antreesy.web@gmail.com> Signed-off-by: S1m <31284753+p1gp1g@users.noreply.github.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com> Signed-off-by: sim <git@sgougeon.fr>
Signed-off-by: sim <git@sgougeon.fr>
Co-authored-by: Maksim Sukharev <antreesy.web@gmail.com> Signed-off-by: S1m <31284753+p1gp1g@users.noreply.github.com> Signed-off-by: sim <git@sgougeon.fr>
|
Sorry, I missed another unconventional commit |
|
Will send a recompile PR afterwards |
This PR uses the new web push support (#2662) to get push notifications on the web application, even when nextcloud isn't opened in the browser.
It achieves 2 things:
To work, this PR needs to add
worker-src 'self'to the CSP by default:https://github.com/nextcloud/server/blob/master/lib/public/AppFramework/Http/EmptyContentSecurityPolicy.php#L62