basic notify_push usage with session handling #3876

alangecker · 2022-06-20T21:33:46Z

Touches: Contents synchronisation / Live update for changes #256
Target version: master

Summary

keeps track of current active sessions on a board and shows them in the UI, including real time updates via notify_push.

it is not very useful in itself, but acts as a base and is probably most of the work towards getting live updates for changes #256 finally, because the server needs to know to whom it sends these updates.

If this step goes through, I'm also happy to work on the actual live updates :)

Part of the code is thankfully taken from @juliushaertl sessions in text

Requirement

it needs notify_push installed and running.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Documentation (manuals or wiki) has been updated or is not required

alangecker · 2022-06-24T14:55:02Z

tests and analysis currently still fail with errors, which seem totally unrelated to my changes.
Is master broken or do I oversee somthing?

ERROR: UndefinedMethod - lib/Service/CirclesService.php:61:21 - Method OCA\Circles\CirclesManager::startSuperSession does not exist
- Other PR's like also fail, so probably not my fault :)

.

1) OCA\Deck\Db\AssignmentMapperTest::testFind
OCA\Deck\NoPermissionException: Creating boards has been disabled for your account.

/home/runner/work/deck/deck/apps/deck/lib/Service/BoardService.php:316
/home/runner/work/deck/deck/apps/deck/tests/integration/database/AssignmentMapperTest.php:98
/home/runner/work/deck/deck/apps/deck/tests/integration/database/AssignmentMapperTest.php:93

Error: Your lock file does not contain a compatible set of packages. Please run composer update.
(didn't modify the compose files)

lib/AppInfo/Application.php

juliushaertl

Thanks a lot for the pull request. This is a great start and I'm very sorry that it took so long to give some feedback. I had a first look at the code and the general approach. I've added some first remarks in the code.

I'll take some time the next days to setup notify_push locally so I can also give this a first test run.

Regarding the test failures, maybe you can rebase your changes. We had some CI failures lately but latest master should pass fine now.

lib/AppInfo/Application.php

lib/Controller/SessionController.php

lib/Db/SessionMapper.php

lib/Event/SessionClosedEvent.php

src/components/Controls.vue

src/components/board/Board.vue

juliushaertl · 2022-07-26T15:33:43Z

Maybe to also kick of some discussion for the next steps, I'm not sure if you have thought about the live update process already, but there is probably a need for an improved approach from the refreshBoard that is currently happening. We already track and expose last modified dates for different elements so that might be an option so that the notify_push queue could advertise the last change date and the vuex store could refresh the content of just the affected items then.

alangecker · 2022-08-04T22:37:00Z

@juliushaertl
thanks for all the feedback 🙂
It is now rebased (hopefully fixing the CI issues) and I resolved some of the smaller points, will continue on it but maybe won't find much time in the next 2 weeks

juliushaertl · 2022-08-05T07:38:09Z

Thanks for the update, looking forward to that :)

matbgn · 2022-10-04T11:41:27Z

@juliushaertl any plan for the review/merge of this highly awaited MR?

alangecker · 2022-10-06T14:32:10Z

I've added now some integration tests and would say that it is now ready to be reviewed from my side 🙂

juliushaertl · 2022-10-06T17:44:09Z

Thanks, I'll have another look next week.

matbgn · 2022-10-28T06:48:11Z

@juliushaertl kindly reminder 😸 ❤️

juliushaertl · 2022-10-28T07:05:49Z

Sorry for the delay, has been busy times 🙈 I'll schedule it next week again

lib/Service/SessionService.php

juliushaertl · 2022-10-29T07:52:17Z

lib/Service/SessionService.php

+ public function notifyAllSessions(IQueue $queue, int $boardId, $event, $excludeUserId, $body) {
+ $activeSessions = $this->sessionMapper->findAllActive($boardId);
+
+ foreach ($activeSessions as $session) {


What would be nice is if we can somehow pass the current session id over on each request from the frontend so that we do not push notifications for the sessions own changes.

Maybe we could wrap axios in some way so that the session (if present) is added as a header to all requests through https://axios-http.com/docs/config_defaults

I've added this now as proposed: ae6774f

It feels a bit hacky, but couldn't think about a more beautiful way. maybe someone has suggestions? 🙂

For the create session and close session events the initiating methods in the Session Service already have the token for the session that caused the event, but I guess for other changes taking the token from a HTTP header is the best approach. Nitpick: I would probably put the burden of finding out the token on the event emitting side, rather than always assuming that the current request is the source of truth. (Think e.g. when a change on board A triggers an automated change on board B in the distant future, then we wouldn't want to set the token from board A.)

lib/Service/SessionService.php

Signed-off-by: chandi Langecker <git@chandi.it>

there is now no regular refreshing anymore, only after update events Signed-off-by: chandi Langecker <git@chandi.it>

Signed-off-by: chandi Langecker <git@chandi.it>

Co-authored-by: Julius Härtl <jus@bitgrid.net> Signed-off-by: chandi Langecker <git@chandi.it>

which resolves the dependency issue: nextcloud-libraries/notify_push-client#5 (during the rebase the commit with an workaround got reverted nextcloud@5aa36aa) Signed-off-by: chandi Langecker <git@chandi.it>

Signed-off-by: chandi Langecker <git@chandi.it>

Signed-off-by: Julius Härtl <jus@bitgrid.net>

juliushaertl · 2023-01-03T14:23:04Z

Pushed a commit to make the static analysis happy. Good to go from my side once CI is green 👍

alangecker · 2023-01-03T14:25:56Z

I've just created an issue for further discussions about the session closing/creation topic #4354
maybe someone is motivated to put some thinking effort into it? 🙃

@juliushaertl nice, big thanks for the commit, reviewing and everything else :)

juliushaertl · 2023-01-03T14:51:52Z

Thanks a lot for your awesome contribution. So great to see the first part being merged. Keep up the great work. ❤️

alangecker force-pushed the sessions branch 3 times, most recently from 717e1da to f4dd246 Compare June 24, 2022 12:38

juliushaertl self-requested a review July 5, 2022 12:27

juliushaertl added enhancement 3. to review labels Jul 5, 2022

juliushaertl reviewed Jul 26, 2022

View reviewed changes

lib/AppInfo/Application.php Outdated Show resolved Hide resolved

juliushaertl reviewed Jul 26, 2022

View reviewed changes

juliushaertl added 2. developing and removed 3. to review labels Jul 26, 2022

alangecker force-pushed the sessions branch 4 times, most recently from 46c168a to f1b655f Compare August 4, 2022 16:11

alangecker force-pushed the sessions branch from 0bccc0e to ec099eb Compare September 26, 2022 14:20

alangecker marked this pull request as ready for review September 26, 2022 21:30

alangecker force-pushed the sessions branch from 4d6ce10 to e2c8ede Compare October 6, 2022 14:28

juliushaertl self-requested a review October 28, 2022 07:06

juliushaertl reviewed Oct 29, 2022

View reviewed changes

lib/Service/SessionService.php Show resolved Hide resolved

juliushaertl reviewed Oct 29, 2022

View reviewed changes

lib/Service/SessionService.php Outdated Show resolved Hide resolved

alangecker and others added 23 commits January 3, 2023 12:43

optimize sql queries / indexing

af13495

Signed-off-by: chandi Langecker <git@chandi.it>

remove refreshData() leftover

0aede22

there is now no regular refreshing anymore, only after update events Signed-off-by: chandi Langecker <git@chandi.it>

fix linter errors after rebase

0272b2d

Signed-off-by: chandi Langecker <git@chandi.it>

avoid naming conflicts for indexes

ed125e9

Signed-off-by: chandi Langecker <git@chandi.it>

docs: sessions API documentation

6684730

Signed-off-by: chandi Langecker <git@chandi.it>

session list: minor visual improvements

c82a5a1

Signed-off-by: chandi Langecker <git@chandi.it>

sessions: integration tests

43a1b1a

Signed-off-by: chandi Langecker <git@chandi.it>

fix acl test

2d465a7

Signed-off-by: chandi Langecker <git@chandi.it>

fix php7 syntax error (unexpected '|')

a674b5b

Signed-off-by: chandi Langecker <git@chandi.it>

Update lib/Service/SessionService.php

54b3900

Co-authored-by: Julius Härtl <jus@bitgrid.net> Signed-off-by: chandi Langecker <git@chandi.it>

Update lib/Controller/SessionController.php

e1e29fe

Co-authored-by: Julius Härtl <jus@bitgrid.net> Signed-off-by: chandi Langecker <git@chandi.it>

Update lib/Controller/SessionController.php

8007a06

Co-authored-by: Julius Härtl <jus@bitgrid.net> Signed-off-by: chandi Langecker <git@chandi.it>

update @nextcloud/notify_push to 1.1.3

b114520

which resolves the dependency issue: nextcloud-libraries/notify_push-client#5 (during the rebase the commit with an workaround got reverted nextcloud@5aa36aa) Signed-off-by: chandi Langecker <git@chandi.it>

stub OCA\NotifyPush\Queue\IQueue

d791949

Signed-off-by: chandi Langecker <git@chandi.it>

session cleanup via background job

0ea384d

Signed-off-by: chandi Langecker <git@chandi.it>

make psalm & linter happy

5386e6e

Signed-off-by: chandi Langecker <git@chandi.it>

sessions: let's be an OCS API to support extenal clients

ccf7373

Signed-off-by: chandi Langecker <git@chandi.it>

sessions: fix failing tests due to changed api endpoints

6d86ec7

Signed-off-by: chandi Langecker <git@chandi.it>

sessions: ignore self-emitted update events

541ee13

Signed-off-by: chandi Langecker <git@chandi.it>

fix syntax error/mistake after rebase

f03521e

Signed-off-by: chandi Langecker <git@chandi.it>

fix broken requests in tests after switch to OCS

e89cd98

Signed-off-by: chandi Langecker <git@chandi.it>

use dependency injection for logger

a466b76

Signed-off-by: chandi Langecker <git@chandi.it>

fix intentation mistake during rebase

a1d2e69

Signed-off-by: chandi Langecker <git@chandi.it>

alangecker force-pushed the sessions branch from 22e52a5 to a1d2e69 Compare January 3, 2023 13:30

alangecker mentioned this pull request Jan 3, 2023

Improvement for session closing/creation? #4354

Open

chore(psalm): Fix annotations to make psalm happy

d892979

Signed-off-by: Julius Härtl <jus@bitgrid.net>

marcelklehr merged commit 5a97b0e into nextcloud:master Jan 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic notify_push usage with session handling #3876

basic notify_push usage with session handling #3876

alangecker commented Jun 20, 2022 •

edited

Loading

alangecker commented Jun 24, 2022 •

edited

Loading

juliushaertl left a comment

juliushaertl commented Jul 26, 2022

alangecker commented Aug 4, 2022 •

edited

Loading

juliushaertl commented Aug 5, 2022

matbgn commented Oct 4, 2022

alangecker commented Oct 6, 2022

juliushaertl commented Oct 6, 2022

matbgn commented Oct 28, 2022

juliushaertl commented Oct 28, 2022

juliushaertl Oct 29, 2022

alangecker Nov 21, 2022

marcelklehr Nov 23, 2022

juliushaertl commented Jan 3, 2023

alangecker commented Jan 3, 2023

juliushaertl commented Jan 3, 2023

basic notify_push usage with session handling #3876

basic notify_push usage with session handling #3876

Conversation

alangecker commented Jun 20, 2022 • edited Loading

Summary

Requirement

Checklist

alangecker commented Jun 24, 2022 • edited Loading

juliushaertl left a comment

Choose a reason for hiding this comment

juliushaertl commented Jul 26, 2022

alangecker commented Aug 4, 2022 • edited Loading

juliushaertl commented Aug 5, 2022

matbgn commented Oct 4, 2022

alangecker commented Oct 6, 2022

juliushaertl commented Oct 6, 2022

matbgn commented Oct 28, 2022

juliushaertl commented Oct 28, 2022

juliushaertl Oct 29, 2022

Choose a reason for hiding this comment

alangecker Nov 21, 2022

Choose a reason for hiding this comment

marcelklehr Nov 23, 2022

Choose a reason for hiding this comment

juliushaertl commented Jan 3, 2023

alangecker commented Jan 3, 2023

juliushaertl commented Jan 3, 2023

alangecker commented Jun 20, 2022 •

edited

Loading

alangecker commented Jun 24, 2022 •

edited

Loading

alangecker commented Aug 4, 2022 •

edited

Loading