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

implementing Push App #17016

Closed
wants to merge 16 commits into from
Closed

implementing Push App #17016

wants to merge 16 commits into from

Conversation

ArtificialOwl
Copy link
Member

@ArtificialOwl ArtificialOwl commented Sep 5, 2019

still working on it...

  • make it work on multiple tabs
  • assign keyword to identify Events so other apps can edit an existing message
  • comments

Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
autoload

Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
some more

Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
autoload

Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
events

Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@ArtificialOwl ArtificialOwl added the 2. developing Work in progress label Sep 5, 2019
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@ArtificialOwl ArtificialOwl added 3. to review Waiting for reviews enhancement and removed 2. developing Work in progress labels Sep 6, 2019
@ArtificialOwl ArtificialOwl added this to the Nextcloud 18 milestone Sep 6, 2019
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Overall looks okay but for my taste quite complex. I think we can remove some of the specific attributes. If we have a topic and payload per event that is pushed, apps can put their meta data and other specifics into the payload, no?

lib/private/Push/Model/Helper/PushCallback.php Outdated Show resolved Hide resolved
*
* @since 18.0.0
*/
public function getMessage(): string {
Copy link
Member

Choose a reason for hiding this comment

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

would "topic" made sense instead of "message"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say message is good, but we could switch to topic. Anyone else would like to share his 2 cents ?

*
* @since 18.0.0
*/
public function getLevel(): string {
Copy link
Member

Choose a reason for hiding this comment

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

why do we need a level and why is it a string?

Copy link
Member Author

Choose a reason for hiding this comment

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

	const LEVEL_SUCCESS = 'success';
	const LEVEL_MESSAGE = 'message';
	const LEVEL_WARNING = 'warning';
	const LEVEL_ERROR = 'error';

It is the 'type' of the notification, but type is already used as the type of the event.
level night not be the best one. I could set notificationType ?

*
* @since 18.0.0
*/
public function setLink(string $link): IPushNotification {
Copy link
Member

Choose a reason for hiding this comment

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

what's this?

Copy link
Member Author

Choose a reason for hiding this comment

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

The url to open if the user click on a displayed notification

Copy link
Member

Choose a reason for hiding this comment

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

is this for a specific use case? what if the notification does not have a link for the user to click?

Copy link
Member Author

Choose a reason for hiding this comment

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

If link is not defined nothing will happens when the user click on the OCA.Toast message. I don't have any use cases for an empty link, but I guess there is some.
However I got multiple use cases of adding a link to a notification:

  • Someone shares you a file, clicking will open the Files App to the right folder,
  • You have a new follower on Nextcloud Social, you click on the notification ('@Someone is following you') will open the social app and displays information about the new follower,
  • new mention in Talk: open the right channel in the app.

Copy link
Member

Choose a reason for hiding this comment

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

I see. But can't we push generic Events to the front-end and let the front-end handle them? Like if I'm already in files it would make more sense if it would just navigate there instead of loading a new page with the content. Or Talk could just switch the channel.

In any case, the getter always returns a string. If we keep the link then it should be nullable.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, that would be interesting! <3
We could even display more details about an event without leaving the page:

  • You don't need to load the full Social app to display information about a new follower; just a popup with a deny/allow button.
  • you receive a DM on the Talk app, you open the discussion on a popup on the front-end without leaving the current page.

Now, when you say generic event, you're talking about a specific class we already have ? or should I create a new Model for this ?

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't but now that you bring that up we could indeed use https://github.com/nextcloud/server/tree/master/lib/public/EventDispatcher for this.

An idea: we add a new BrowserEvent class that extends Event. An app can then dispatch this event via IEventDispatcher. The push app can listen to that event type and deliver the data to the front end.
On the front-end we then need some mechanism for apps to listen for (server) events. There they can do their magic.
Again, we should probably move this discussion into a ticket.

*
* @since 18.0.0
*/
public function getTtl(): int {
Copy link
Member

Choose a reason for hiding this comment

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

is a TTL mandatory?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a default one, if not set. Why this question ?

Copy link
Member

Choose a reason for hiding this comment

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

yes, but does every notification need to have a TTL? this sounds very specific. the TTL can also be added to the custom payload and when the recipient processes the notification it can ignore if the TTL has passed

Copy link
Member Author

@ArtificialOwl ArtificialOwl Sep 11, 2019

Choose a reason for hiding this comment

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

The TTL is applied before sending the payload to the front-end. You generate a PushItem for a certain amount of seconds and if the targeted user is connected on the Nextcloud before the end of TTL, he will receive the PushItem. If not, it will be delete from the database without being published.

This is executed on the very first polling request when a new page is being displayed on user's browser: https://github.com/nextcloud/push/blob/master/lib/Db/PushRequest.php#L190-L198

Copy link
Member

Choose a reason for hiding this comment

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

I know what TTL stands for and where it's used ;)

My consideration still applies

the TTL can also be added to the custom payload and when the recipient processes the notification it can ignore if the TTL has passed

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get the issue with setting the TTL, we should talk about this in Berlin !

Copy link
Member

Choose a reason for hiding this comment

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

There are notifications where a TTL makes sense, with others it doesn't.

So in general I would not add it. And if it's needed then add it to the payload.

lib/public/FullTextSearch/Model/ISearchResult.php Outdated Show resolved Hide resolved
interface IPushCallback extends IPushRecipients {


const TYPE = 'Callback';
Copy link
Member

Choose a reason for hiding this comment

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

unused

Copy link
Member Author

Choose a reason for hiding this comment

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

it is used by the Push App to define the type of an event

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It is used in the front-end, so I wanted to set them as a static const.

Also, your link is from the PushHelper, that is supposed to help developers to generate basic PushItem, but with the PushService you can generate custom PushItem and set the type to one of the ::TYPE available.

Copy link
Member

Choose a reason for hiding this comment

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

But how can the front-end use these constants? 🙈

interface IPushEvent extends IPushRecipients {


const TYPE = 'Event';
Copy link
Member

Choose a reason for hiding this comment

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

unused

Copy link
Member Author

Choose a reason for hiding this comment

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

it is used by the Push App to define the type of an event

interface IPushNotification extends IPushRecipients {


const TYPE = 'Notification';
Copy link
Member

Choose a reason for hiding this comment

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

unused

Copy link
Member Author

Choose a reason for hiding this comment

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

it is used by the Push App to define the type of an event

interface IPushItem {


const TTL_INSTANT = 15;
Copy link
Member

Choose a reason for hiding this comment

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

let's keep our API minimal and leave that to apps

Copy link
Member Author

@ArtificialOwl ArtificialOwl Sep 11, 2019

Choose a reason for hiding this comment

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

but, this would mean that 3rd party apps would need to get the value from a const defined in OCA\Push... ?

Copy link
Member

Choose a reason for hiding this comment

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

no. they can use whatever int value they want. or do you want to limit the allowed values?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes ! any int would work, that's why the const are set in seconds; but I like the idea of having preset values (as const). Don't you ?

It makes the code more readable, and might guide the developer to set a better value to fit the expected result.

Should I set the const in the class PushItem instead of in the interface ?

Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@ChristophWurst
Copy link
Member

My concern: a lot of this is very specific. We should aim for a general purpose push mechanism. You can always get more specific with what you push, but if we enforce certain attributes they always have to be used. And removing stuff from a public API is always a breaking change.

So, without going into too much detail with the implementation, couldn't we just have a simple push system that pushes generic events to the front-end? And app can then push specific events for notifications, callbacks or anything else. Would that make sense?

I wouldn't mind if we discuss this in a ticket. Then this PR can stay clean.

@go2sh
Copy link
Contributor

go2sh commented Sep 11, 2019

I had a quick look at this code and the app code and my concern is, that this might not scale well. Depending on the instance size and amount of items generated, this puts a huge load on the database, I think. Would it be more beneficial to have different approaches depending on the scaling? I have protocols like AMPQ and MQTT with servers like RabbitMQ in mind.

@ChristophWurst
Copy link
Member

That was also my consideration before this PR even started. There should be a uniform interface for push messages. But DB/polling should only be the fallback for small instances. There should also be the possibility to plug in a websocket service or similar.

@ArtificialOwl
Copy link
Member Author

There should also be the possibility to plug in a websocket service or similar.

Yes, I think we already talked/agreed that it should be scale-able using websockets/nodejs/socket.io

@ArtificialOwl ArtificialOwl removed this from the Nextcloud 18 milestone Dec 5, 2019
@rullzer rullzer added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 24, 2020
@rullzer
Copy link
Member

rullzer commented Mar 30, 2021

I'm going to close this due to lack of activity.
Feel free to reopen if anybody wants to continue.

@rullzer rullzer closed this Mar 30, 2021
@rullzer rullzer deleted the stratos branch March 30, 2021 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants