misc push-related fixes and improvements

[taoeffect]

Related to issues #2647 and #2653

[cypress]

group-income    Run #4062

Run Properties:   Passed #4062  •  89832ef35f ℹ️: Merge 9af8fa7ee313bf13a6bc92a21592f2d53589ef00 into 722036bccbf186eda94a89354682...

Project	group-income
Branch Review	2647-fix-push
Run status	Passed #4062
Run duration	11m 28s
Commit	89832ef35f ℹ️: Merge 9af8fa7ee313bf13a6bc92a21592f2d53589ef00 into 722036bccbf186eda94a89354682...
Committer	Greg Slepak
View all properties for this run ↗︎

---

Test results
Failures	0
Flaky	0
Pending	10
Skipped	0
Passing	114
View all changes introduced in this branch ↗︎

[corrideat]

I've added comments where appropriate, mostly explaining why certain things are the way they were. I'm not too happy (meaning feeling neutral to negative) with the changes made to the frontend management logic (other than the extra logging and the bugfix for reported, using reported.endpoint, which I feel positive about), but it does look like the Vue components had some improvements.

However, I don't think these changes will help much with the issue at hand, namely notifications stopping working after an update. This is why:

1. On Slack, we've discussed these various 401 errors. I think getting to the bottom of this is what will fix the issue, or at least, it's a necessary condition.
2. You seem to be making an effort at increasing the reliability of reporting the subscription to the server. Without focusing on whether the changes accomplish this or not, doing so will likely have little effect (*) on the issue at hand. When a subscription is first reported, the server stores it, and, after that, subsequent failures to report it again (e.g., new WS connections) would result in unnecessary push events being sent by the server, rather than fewer. This is because reporting a subscription actually causes the server to stop sending push events until that WS connection is closed. So, even if the reporting reliability is improved, the issue of notifications not being seen more than likely lies on the server.

(*) There's one exception to this, but it still requires an error on the server. That is, if the logic for storing subscriptions on the DB is faulty, and some or all subscriptions are lost.

[taoeffect]

@corrideat I've addressed all remaining feedback, please check latest changes!

Note: I wasn't able to join groups when creating a link in FF and then joining in Safari.

Also in both Safari and Brave I noticed this upon creating a group:

However upon dismissing I saw that I was joined.

I don't think these issues have anything to do with this PR but might be related to some of that commented out code (#2653) or other issues. Opened an issue for this: #2654

[corrideat]

I realise this was like this from before, but I'm wondering if maybe the order of these two operations should be reversed

[taoeffect]

I don't know, if there's a strong reason for it that can be given I'm certainly happy to do it.

[corrideat]

The reason is that if the deletion fails, then it'll still be in the index.

[taoeffect]

How will reversing the order fix that? It first deletes from the index right now. If there's a problem with writing to the DB, it's the index deletion that will fail first, and then it won't get to deleting the subscriptionID.

Whereas, if it successfully deletes the subscription key, but then fails to delete it from the index, it will be in the index but won't exist.

[corrideat]

It doesn't 'fix' it, but it being in the index but not existing is worse than the converse, based on the assumption that keys may not be enumerable.

[corrideat]

Stray debug?

[taoeffect]

I think it's OK to leave here just in case someone is debugging this part of the code. I found it useful and that's why I decided to comment it out instead of removing it completely. If you really want me to remove it I can though.

[corrideat]

Not a strong feeling either way, but the reason I was not checking for window.Cypress (and generally avoid doing so) is that these are runtime checks (code shipped) while process.env.CI is exclusively a buildtime check (no code shipped).

[taoeffect]

Had to add it because without it the tests weren't passing. An alternative would be to create a Cypress envvar in Gruntfile.js...

[corrideat]

I've been reviewing this and testing it, and it seems like most things work. I didn't yet test this as comprehensively as the previous code (esp. re. the frontend parts), but it looks like it should work.

Can you as a last change request:

1. Fix the order for deleting subscriptions before updating the index
2. Revert the change to shared/domains/chelonia/chelonia.js
3. Save this setting to IndexedDB in setNotificationEnabled and use it in the relevant places. Actually, the Vuex setting may not be needed at all if using IndexedDB.