vaadin / collaboration-engine

The simplest way to build real-time collaboration into web apps
https://vaadin.com/collaboration
Other
3 stars 1 forks source link

Make it easier to integrate system messages #33

Closed emarc closed 2 years ago

emarc commented 3 years ago

Is your feature request related to a use case? Please describe. I want non-user "system" to participate in collaborations, e.g to add system messages to an activity log, or send various events via the low-level API.

Describe the solution you'd like This is currently doable, but difficult since there is no documentation. The biggest hurdle is to figure out how to provide a ConnectionContext when there is no UI / Component. The demo has a EagerConnectionContext; something like this could be included and documented.

Describe alternatives you've considered I stole the EagerConnectionContext from the demo, after getting a tip that demo bots do the same.

heruan commented 3 years ago

A context for the system-user would be a context that is always active, so maybe we can provide a singleton "system-context" instance to be used in those cases. Something like:

ConnectionContext systemContext = CollaborationEngine.getSystemContext();

CollaborationEngine.getInstance().openTopicConnection(systemContext, ...);

Then, there may be also a method in CollaborationMessageList to append system-messages easily:

CollaborationMessageList list = new CollaborationMessageList(user, "topic-123");

list.appendSystemMessage("Hello, world!"); // <- will use the system-context and the system-user
Peppe commented 3 years ago

I really like the suggestion to have the API on the list. There should definitely be an API like you suggested in list.appendSystemMessage("Hello, world!");.

The system context suggestion is also good, but the biggest blocker is how you would know to call that, E.g. EagerConnectionContext worked for @emarc, but he didn't know about it before he was pointed to it. Could we have a variant of openTopicConnection that does not take an context, and would use system context in that case?

Legioth commented 3 years ago

I think it's better to have an explicit "system context" or such rather than allowing openTopicConnection without providing a context. My reason here is that we don't want to make it too easy to accidentally use the system context just by omission. CollaborationEngine.getSystemContext() leads to a small hurdle for discoverability, but it has the benefit that it's less effort to use a component for the connection context in all the cases where a component instance is already readily available. The means for actually submitting system messages does otherwise also require some documentation so it would be quite natural to also cover the part about acquiring a suitable connection context in the same documentation. One thing that might make sense is to offer a shorthand of e.g. openTopicConnection that takes a SystemConnectionContext instance and then automatically uses SystemUserInfo.getInstance() rather than requiring that as a separate parameter.

It seems slightly problematic to tie this functionality to the CollaborationMessageList component API since the part of the system that generates system messages might not be associated with any specific UI instance. Instead, I think the MessageCollaborationDao API suggested in https://github.com/vaadin/collaboration-engine/issues/36 would be very appropriate also for this case. I don't know if it's necessary to have a separate method for appending a system message, or if it's easy enough to create and submit messages with the SystemUserInfo user?

Putting those ideas together, sending a system message to a topic could then be along these lines:

MessageCollaborationDao dao = new MessageCollaborationDao(SystemConnectionContext.getInstance(), "someTopic", samePersisterAsAlways);
dao.sendMessage("Hello, world!");

The DAO instance should preferably be created only once and reused every time a new system message is sent. Otherwise, we might also want to support using the DAO with try-with-resources, to automatically close it when it's no longer used:

try (MessageCollaborationDao dao = ...) {
  dao.sendMessage("Hello, world");
}
emarc commented 3 years ago

Just some additional nuance in case it helps: Maybe the main point here is that it is a non-UI participant? Integrations with other services or maybe a helpful AI (think Slack bots) might need different "users", but they do not have a UI instance. These needs may be addressed on another level of abstraction, but maybe it's useful to keep in mind?

Legioth commented 3 years ago

Sounds like it would make sense to have an overload of sendMessage that also accepts a UserInfo for such purposes.

It's also an interesting question whether those kinds of "users" should be counted towards the license quota?

gtzluis commented 3 years ago

It's also an interesting question whether those kinds of "users" should be counted towards the license quota?

I'd expect that at most all bot-users should be counted as 1 user towards the license quota, or not counted at all.

Legioth commented 2 years ago

When using SystemConnectionContext, we should ensure that UI.getCurrent() and VaadinSession.getCurrent() are not defined during the dispatch to avoid accidentally leaking context from something that triggered the change that caused the system context to be invoked. This can be implemented using getInstances, clear and restoreInstances from CurrentInstance.

In the same way, we should also ensure VaadinService.getCurrent() is defined even if the trigger is from outside any Vaadin context.

We might also want to provide a pluggable way of dispatching system tasks in a way that can preserve e.g. the Spring application context or other similar thread locals.

heruan commented 2 years ago

Closed in https://github.com/vaadin/collaboration-engine-internal/pull/663