Skip to content

Conversation

@JammingBen
Copy link
Contributor

Description

Add a proper translation in the activity stream for shares which expired automatically. Previous to this, the expiry was authored by a user, which is technically not true.

Related Issue

Screenshots (if appropriate):

image

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

@JammingBen JammingBen marked this pull request as ready for review April 9, 2021 11:34
@JammingBen JammingBen requested a review from jvillafanez April 9, 2021 11:34
@jvillafanez
Copy link
Member

I'm not sure if this is right. Technically, the agent is like a virtual user that performs the action on behalf of another user.

Having a "share_expire_author" doesn't fit here. What if there are more actions triggered during the share expiration? They could have the "share_expire_author" for a delete file action, which seems weird. Also note that whoever is listening for the delete file event won't likely take into account a "share_expire_author" agent, or won't receive any special treatment.

I'd prefer to reuse the "automation_author" in this case and try to figure out if the share has expired in a different way. A "cli_author" seems to generic for this case, but we could use a different author.
The question is who performs the share expiration and any other related action that could happen during that time.

@JammingBen
Copy link
Contributor Author

JammingBen commented Apr 9, 2021

@jvillafanez

I'd prefer to reuse the "automation_author" in this case and try to figure out if the share has expired in a different way.

What do you think of expanding the event parameters here by shareExpired:

$hookParams = [

This way we could use this information in the activity app and add it as param. The author could be automation_author then.

@jvillafanez
Copy link
Member

That's fine

@JammingBen
Copy link
Contributor Author

@jvillafanez I implemented it the way you suggested. Some changes are required in the activity app for this, see owncloud/activity#944. We basically now use the automation author and the shareExpired param to indicate if a share expired.

@AlexAndBear AlexAndBear self-requested a review April 12, 2021 09:44
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

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

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

@JammingBen JammingBen merged commit 188db4c into master Apr 12, 2021
@delete-merged-branch delete-merged-branch bot deleted the issues/share-expiry-activity-info branch April 12, 2021 13:02
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.

4 participants