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

Automations in activity stream #38605

Merged
merged 14 commits into from
Apr 8, 2021
Merged

Conversation

AlexAndBear
Copy link

@AlexAndBear AlexAndBear commented Mar 31, 2021

Description

This adjustment provides a way to handle the activity stream for actions that were triggered by an automation (like the workflow app e.g.). As the workflow app currently only handles delete and assign_tag, this is the only action we take care of for now.

The changes go together with implementations in the workflow and the activity app, although they do not rely on them.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@AlexAndBear AlexAndBear force-pushed the workflow-activity-enhancements branch 2 times, most recently from c11edc3 to 081b515 Compare March 31, 2021 08:06
@AlexAndBear AlexAndBear force-pushed the workflow-activity-enhancements branch from 081b515 to d956cdd Compare March 31, 2021 08:24
@JammingBen JammingBen changed the title enhancement Automations in activity stream Mar 31, 2021
@JammingBen JammingBen marked this pull request as ready for review March 31, 2021 11:49
lib/public/Activity/DataProvider.php Outdated Show resolved Hide resolved
apps/systemtags/lib/Activity/Extension.php Outdated Show resolved Hide resolved
@jvillafanez
Copy link
Member

It seems that whether the activity comes from a user or from something automated should be bound to the event itself. The $event->setAuthor(...) could be an idea, and it seems more consistent.

I guess the problem of using $event->setAuthor(null) is with oracle storing an empty string in the DB, which could cause problems. I'm not sure if we can store something like "user:myuser" or "auto:cli" in the DB so we can distinguish these cases.
We can have some constants so we can do $event->setAuthor(IEvent::AUTHOR_CLI) and $event->getAuthor() === IEvent::AUTHOR_CLI so it's a bit easier to deal with this from the outside.
I'm not sure if it will be easy to keep compatibility, but I think it's worth a shot.

@AlexAndBear
Copy link
Author

AlexAndBear commented Apr 1, 2021

@jvillafanez This was our idea at first, but the event ist triggered inside for example in the node->delete function, we could assign the delete function a metaObject paramerer, which will be passed trough the event, BUT this needs a change to the Interface and all classes which implements it (and this means required changes in a Lot of the Apps)
Any Ideas Welcome!

Btw AUTHOR_CLI does Not Match in all Cases, for example Auto Tagging will Not be processed via cli

@jvillafanez
Copy link
Member

The problem with the tags seems to be in https://github.com/owncloud/core/blob/master/apps/systemtags/lib/Activity/Listener.php#L88-L93 . Assuming that the author is the user in the session is wrong. We'll likely need to change the ManagerEvent and get the author from there. We can assume the author is the user in the session only as a fallback.
I haven't traced all the code, so there could be more issues to go around.

FS actions seem to be more complex since they're using the old hooks and not the symfony's events, so we can't enrich the events with additional information.


An alternative to explore could be around

if ($event->getAuthor() === null) {
if ($this->session !== null && $this->session->getUser() instanceof IUser) {
$event->setAuthor($this->session->getUser()->getUID());
}
}
.
We could add a setDefaultAuthor(....) in the activity manager. Whenever someone wants to publish an activity without an explicit user, we can get the default author and override the event.
The idea is to set the default author to the session user by default in order to keep compatibility. Now, for our case, we could change this default author to a different one, perform the action, and then restore the old default author. We'll have to check if this works or not.

We'll still need something like #38605 (comment) mainly to prevent collisions with valid usernames.

@AlexAndBear
Copy link
Author

@jvillafanez we save now the automated user in the database which works fine, the thing is we did not understand how you want to achieve saving the user not by the session (we still go by the global var, just changed the location), can you create a PR and show us, what you mean exactly?

lib/private/Activity/Manager.php Outdated Show resolved Hide resolved
lib/public/Activity/IEvent.php Outdated Show resolved Hide resolved
lib/private/Activity/Manager.php Outdated Show resolved Hide resolved
lib/private/Activity/Manager.php Outdated Show resolved Hide resolved
@jvillafanez
Copy link
Member

@jvillafanez we save now the automated user in the database which works fine, the thing is we did not understand how you want to achieve saving the user not by the session (we still go by the global var, just changed the location), can you create a PR and show us, what you mean exactly?

We're overriding the author in the publish method if the event doesn't provide an author. Instead of using the global variable, we just set the default author in the activity manager, perform the action, and then restore the previous default author. The idea is roughly the same of using the global variable, but without using a global variable.
If the default author is null, we can still override the author with the user in the session, with the same rules. This will keep the code compatible with what we have now.

@AlexAndBear
Copy link
Author

@jvillafanez the automated tagging provides the session with the current user, so we still have a user in the session

@JammingBen
Copy link
Contributor

@jvillafanez I renamed the methods and tried to explain the usage in the interface docs.

Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

Couple of things and I think we're on this side

apps/files/lib/Activity.php Outdated Show resolved Hide resolved
apps/systemtags/lib/Activity/Listener.php Outdated Show resolved Hide resolved
@jvillafanez
Copy link
Member

jvillafanez commented Apr 7, 2021

After checking the PR for the workflow app, I've remember why I wanted to bind the "agent" author to the event: I expect a usage such as:

$activityManager->setAgentAuthor(IEvent::AUTOMATION_AUTHOR);
doSomething();
$activityManager->setAgentAuthor(null);

This way, the scope is clear and limited to the doSomething method.

The problem I see with the current approach is that we lose control over when we can reset the agent. Other actions that could happen after that doSomething might be misinterpreted as being automated because we didn't reset the agent. In addition, I don't think it's a good idea to rely on the activity app to reset the agent.

@JammingBen
Copy link
Contributor

I honestly don't see a way to keep $activityManager->setAgentAuthor(IEvent::AUTOMATION_AUTHOR); and $activityManager->setAgentAuthor(null); together. Let's look at the automated delete:

We have to set $agentAuthor to IEvent::AUTOMATION_AUTHOR within our workflow app: https://github.com/owncloud/workflow/blob/87066fdef98a94afd7a624d7c0d51f68c37fc5ee/lib/Retention/TagBasedRetention.php#L295. This is the beginning of our "trace". There is no other place to do that besides passing a bunch of parameters to $node->delete(). Setting the agent author back to null immediately afterwards also wont work as it would be null by the time the event is being handled.

Binding the agent author to the event also does not work because we don't have an event yet at this point.

@jvillafanez
Copy link
Member

Binding the agent author to the event also does not work because we don't have an event yet at this point.

When a new event is published, the activity manager should override the author of the event with the agent. Basically, with the setAgentAuthor you tell the activity manager to set that author for any event that will be published, so you can restore the previous agent just after the $node->delete(). Basically, the workflow app just hints the activity manager, but it's the activity manager the one binding the agent with the event.
Did we reject this idea? There was some code previously, but I think we just shifted the approach.

@JammingBen
Copy link
Contributor

JammingBen commented Apr 7, 2021

Hmm yeah, restoring the agent directly after $node->delete() works. Strange, I'm pretty sure we ran into other issues when we tried this first 🤔 Although I'm slowly getting confused about all the changes and versions we had...

Please have a look again. This PR did not change (besides minor adjustments regarding your latest comments), but those for workflow and activity did. Now we've properly wrapped the calls this way:

$activityManager->setAgentAuthor(IEvent::AUTOMATION_AUTHOR);
doSomething();
$activityManager->setAgentAuthor(null);

Reseting the agent author in the activity app was removed therefore.

@jvillafanez
Copy link
Member

We might need to include a stack or similar to support nested contexts. Expected usage could be:

$activityManager->setAgentAuthor('cli');
doSomething();
$activityManager->setAgentAuthor('automation');
doAnotherThing();
$activityManager->restoreAgentAuthor();
doAthirdThing();
$activityManager->restoreAgentAuthor();

Both doSomething and doAThirdThing method should be authored by the cli, while the doAnotherThing should be authored by the automation. Note that the calls will likely be in different places.

A valid example could be a command in the workflow app. We'll likely have to add the cli as author at some point

@JammingBen
Copy link
Contributor

@jvillafanez Could you get us a hint on how to achieve that?

@jvillafanez
Copy link
Member

You can have an array and use it as a stack. That should be easy to implement if we don't want to create a class just for this:

private $agentStack = [];

public function setAgentAuthor($author) {
  $this->agentStack[] = $author;
}

public function getAgentAuthor() {
  return $this->agentStack[\count($this->agentStack) - 1];
}

public function restoreAgentAuthor() {
  return \array_pop($this->agentStack);
}

I'm pretty sure there are some things to take care, such as getting or restoring an agent when the stack is empty, but that should be most of it. This should be transparent to the apps.
You can also document the functions properly to make the code as easy as possible. For example, the restoreAgentAuthor could return the last agent set (after resetting the agent) or null if there is no agent set (because array_pop will return null if the array is empty)

@JammingBen
Copy link
Contributor

Alrighty, I applied your suggestion. Also updated the docs in the interface regarding its usage.

@AlexAndBear
Copy link
Author

AlexAndBear commented Apr 8, 2021

@jvillafanez can you have a Look again over the whole implementation, we should really come to an end here.

lib/private/Activity/Manager.php Show resolved Hide resolved
lib/public/Activity/IManager.php Outdated Show resolved Hide resolved
@jvillafanez
Copy link
Member

Assuming it works, I guess those are the last changes here.

@jvillafanez
Copy link
Member

Do we want to merge this PR now, or do we freeze it until the rest of the pieces are ready? I don't think we'll need to make changes here, but you never know

@jvillafanez
Copy link
Member

BTW, we should include some unit tests for these changes. The ones for the agent stuff should be pretty easy.

@AlexAndBear
Copy link
Author

Rest of the pieces? This PR needs to merged to get CI Green for activity and Workflow. As well another issue ist waiting, which relies on this

@jvillafanez
Copy link
Member

Rest of the pieces? This PR needs to merged to get CI Green for activity and Workflow. As well another issue ist waiting, which relies on this

The PR for the workflow app should use the restoreAgentAuthor instead of setting a null agent. That's why I'm asking.

I'm ok with merging this.

@JammingBen
Copy link
Contributor

Added a simple unit test for setting, getting and restoring an agent author and also adjusted the workflow app.

@sonarcloud
Copy link

sonarcloud bot commented Apr 8, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
No Duplication information No Duplication information

@JammingBen JammingBen merged commit 38af92c into master Apr 8, 2021
@delete-merged-branch delete-merged-branch bot deleted the workflow-activity-enhancements branch April 8, 2021 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants