feat(files_sharing): reminder for link shares with expiration date #47412

Luka-sama · 2024-08-22T07:40:06Z

Resolves: Reminder for link shares with expiration date #45676

Summary

This pull request implements the feature as described in the associated issue. A cron job runs hourly to check if any empty folders have less than a day remaining before their expiration date. The feature supports two types of shares: user shares and email shares.

Additionally, a migration was added to include a 'reminder sent' flag, ensuring that reminders are neither skipped nor sent multiple times when the job runs at irregular intervals, especially when using webcron or ajax instead of cron.

This solution can be easily extended to send reminders for link shares without an expiration date (e.g. after 4 days), if the maintainers choose to implement this feature.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

come-nc · 2024-08-22T10:12:29Z

apps/files_sharing/lib/SharesReminderJob.php

+		$qb = $this->db->getQueryBuilder();
+		$qb->select('id', 'share_type')
+			->from('share')
+			->where(
+				$qb->expr()->andX(
+					$qb->expr()->orX(
+						$qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_USER)),
+						$qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_EMAIL))
+					),
+					$qb->expr()->eq('item_type', $qb->expr()->literal('folder')),
+					$qb->expr()->gte('expiration', $qb->createNamedParameter($minDate->format('Y-m-d H:i:s'))),
+					$qb->expr()->lt('expiration', $qb->createNamedParameter($maxDate->format('Y-m-d H:i:s'))),
+					$qb->expr()->eq('reminder_sent', $qb->createNamedParameter(
+						false, IQueryBuilder::PARAM_BOOL
+					))
+				)
+			);


Should this go into a mapper or manager of some kind? I know in other apps we usually do not hit the DB directly like that, not sure for shares.

I could move this to the share manager, but I'm not sure if I should, since the query is very specific to this particular job 🤔

apps/files_sharing/appinfo/info.xml

come-nc · 2024-08-22T10:21:20Z

apps/files_sharing/lib/SharesReminderJob.php

+			$qb->update('share')
+				->where($qb->expr()->eq('id', $qb->createNamedParameter($share->getId())))
+				->set('reminder_sent', $qb->createNamedParameter($share->getReminderSent()))
+				->execute();


Suggested change

->execute();

->executeStatement();

But again, I’d like to see that moved somewhere else.
The updateShare method seems a bit much for such a small flag change though, so I don’t know 🤔

I think updateShare works well here, the overhead (if any) is quite insignificant.

come-nc · 2024-08-22T10:22:09Z

lib/private/Share20/Share.php

+	/** @var bool */
+	private $reminderSent = false;


Suggested change

/** @var bool */

private $reminderSent = false;

private bool $reminderSent = false;

Should I maybe update other properties to this format as well (to make it consistent)?

Should I maybe update other properties to this format as well (to make it consistent)?

I decided against it because it breaks a lot of code (e.g. some int properties are sometimes set to null). So I did the strict typing only for my property.

lib/private/Share20/Share.php

come-nc · 2024-08-22T10:26:47Z

apps/files_sharing/lib/SharesReminderJob.php

+	public function run(mixed $argument): void {
+		$shares = $this->getShares();
+		[$foldersByEmail, $langByEmail] = $this->prepareReminders($shares);
+		$this->sendReminders($foldersByEmail, $langByEmail);
+	}


You need to put a limit here, and work only on a chunk of possible reminders, like 1000. Otherwise you can run into memory issues and such.
Also, you are first gathering all the info and then sending all the email, consider using Generators to send shares as you find them.
Also, you need to consider what happens if your job crashes or is stopped mid-process. In the current state it sets the flag reminder_sent on all shares first, which means it won’t get sent for other shares if it crashes on a share before. This is better than re-sending reminders, but it would still be better to flag shares one by one just before sending the email.

Fixed, thank you!

skjnldsv · 2024-08-22T10:40:23Z

Great idea @Luka-sama !!
Take your time to address Come's comments 👍

Luka-sama · 2024-08-22T14:57:22Z

Thank you @come-nc and @skjnldsv! The idea comes from the issue, but I found it a very interesting task to implement it here :)
That's a very nice idea with generators! I will fix/address this and other problems in the next days.

Luka-sama · 2024-08-27T07:44:12Z

So, I've fixed the problems that @come-nc found!
To do this, I moved the condition checking (write permissions and folder emptiness) to the database query - to avoid the situation where the job checks the same 1000 shares every time.

come-nc

reminder_sent should be a bool in DB as well, otherwise good.
Only thing a bit questionable is how would other providers add support for this, but this can be refactored in later.

apps/files_sharing/lib/SharesReminderJob.php

apps/sharebymail/lib/ShareByMailProvider.php

Luka-sama · 2024-09-02T13:18:22Z

Thank you for your code review!
Yes, currently only two types of shares are supported (both in providers and in the job itself). However, support for other types can easily be added if needed.

come-nc

What I meant was that this could’ve been added in a more extensible way, for instance with an interface for providers which want to implement the feature, and which adds a method to get which email address the reminder should be sent to.
Just thinking out loud, I have no idea if this would’ve been a good idea and all it implies, just a suggestion for future work to think about extensibility by applications.

Approved 👍

apps/files_sharing/lib/SharesReminderJob.php

+				'token' => $share->getToken()
+			]);
+		}
+		if (empty($reminderInfo['email'])) {


Luka-sama · 2024-09-02T20:04:11Z

You're right, that's definitely a good idea for the future to make the feature more universal this way.

Thanks!

Signed-off-by: Stefan Cherniakov <[email protected]>

skjnldsv · 2024-09-03T11:37:40Z

Ok, let's go! 🚀

…int for reminder_sent field Signed-off-by: Stefan Cherniakov <[email protected]>

Signed-off-by: Stefan Cherniakov <[email protected]>

welcome · 2024-09-03T15:24:05Z

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

Pytal · 2024-09-03T18:18:58Z

This broke CI on master cc @skjnldsv

susnux · 2024-09-04T22:21:10Z

apps/files_sharing/lib/Migration/Version31000Date20240821142813.php

+use OCP\Migration\IOutput;
+use OCP\Migration\SimpleMigrationStep;
+
+class Version31000Date20240821142813 extends SimpleMigrationStep {


As this target 31 it should have a migration annotation, otherwise we have no information of the impact.

I have added it:
d758442
However, my changes are no longer displayed here in this pull request, probably because the branch is already merged into master.

@Luka-sama yes because it is already merged. But is also not about a doc-comment but about annotation for the kind of the migration (OCP\Migration\Attributes\MigrationAttribute).
Nevertheless thank you very much!!

@susnux Oh, I see! I must have used the older migrations as an example, the attributes seem to be something new (from the 30th version).
Should I add them too? Except that it would probably be an overkill to make a new pull request just for that 😅 (or not?)

github-actions · 2024-09-06T01:55:09Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Luka-sama requested review from skjnldsv and come-nc as code owners August 22, 2024 07:40

Luka-sama force-pushed the feat/shares-reminder branch 2 times, most recently from 3deeb2c to 86be354 Compare August 22, 2024 07:48

come-nc reviewed Aug 22, 2024

View reviewed changes

skjnldsv added 2. developing Work in progress feature: sharing feature: emails labels Aug 22, 2024

skjnldsv added this to the Nextcloud 31 milestone Aug 22, 2024

Luka-sama requested a review from come-nc August 27, 2024 07:45

come-nc reviewed Sep 2, 2024

View reviewed changes

apps/files_sharing/lib/SharesReminderJob.php Outdated Show resolved Hide resolved

apps/sharebymail/lib/ShareByMailProvider.php Outdated Show resolved Hide resolved

come-nc approved these changes Sep 2, 2024

View reviewed changes

github-advanced-security bot found potential problems Sep 2, 2024

View reviewed changes

Luka-sama force-pushed the feat/shares-reminder branch from 23ee40d to 3e91bb7 Compare September 2, 2024 19:59

skjnldsv approved these changes Sep 3, 2024

View reviewed changes

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Sep 3, 2024

Luka-sama added 2 commits September 3, 2024 13:37

feat(files_sharing): reminder for link shares with expiration date

5e4a166

Signed-off-by: Stefan Cherniakov <[email protected]>

fix(files_sharing): Make share reminders more stable & fix issues

d633b9b

Signed-off-by: Stefan Cherniakov <[email protected]>

skjnldsv force-pushed the feat/shares-reminder branch from 3e91bb7 to f6d05d0 Compare September 3, 2024 11:37

Luka-sama added 2 commits September 3, 2024 17:23

fix(files_sharing): Add missing check for null & use bool instead of …

457eaee

…int for reminder_sent field Signed-off-by: Stefan Cherniakov <[email protected]>

fix(files_sharing): Add check for null

2685501

Signed-off-by: Stefan Cherniakov <[email protected]>

skjnldsv force-pushed the feat/shares-reminder branch from f6d05d0 to 2685501 Compare September 3, 2024 15:23

skjnldsv merged commit f9fcc5b into nextcloud:master Sep 3, 2024
42 of 44 checks passed

AndyScherzinger added 🍀 2025-Spring enhancement labels Sep 4, 2024

susnux reviewed Sep 4, 2024

View reviewed changes

github-actions bot added the feedback-requested label Sep 6, 2024

susnux mentioned this pull request Sep 6, 2024

chore(files_sharing): Add migration attribute for share.reminder_sent column #47803

Merged

4 tasks

icewind1991 mentioned this pull request Sep 26, 2024

fix: fix share reminder job for oracle #48182

Merged

skjnldsv mentioned this pull request Jan 7, 2025

31.0.0 beta 1 #50066

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(files_sharing): reminder for link shares with expiration date #47412

feat(files_sharing): reminder for link shares with expiration date #47412

Luka-sama commented Aug 22, 2024

come-nc Aug 22, 2024

Luka-sama Aug 22, 2024

come-nc Aug 22, 2024

Luka-sama Aug 27, 2024

come-nc Aug 22, 2024

Luka-sama Aug 22, 2024

Luka-sama Aug 27, 2024

come-nc Aug 22, 2024

Luka-sama Aug 27, 2024

skjnldsv commented Aug 22, 2024

Luka-sama commented Aug 22, 2024

Luka-sama commented Aug 27, 2024

come-nc left a comment

Luka-sama commented Sep 2, 2024

come-nc left a comment

Luka-sama commented Sep 2, 2024

skjnldsv commented Sep 3, 2024

welcome bot commented Sep 3, 2024

Pytal commented Sep 3, 2024

susnux Sep 4, 2024

Luka-sama Sep 5, 2024

susnux Sep 5, 2024

Luka-sama Sep 5, 2024

github-actions bot commented Sep 6, 2024

	/** @var bool */
	private $reminderSent = false;
	private bool $reminderSent = false;

feat(files_sharing): reminder for link shares with expiration date #47412

feat(files_sharing): reminder for link shares with expiration date #47412

Conversation

Luka-sama commented Aug 22, 2024

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjnldsv commented Aug 22, 2024

Luka-sama commented Aug 22, 2024

Luka-sama commented Aug 27, 2024

come-nc left a comment

Choose a reason for hiding this comment

Luka-sama commented Sep 2, 2024

come-nc left a comment

Choose a reason for hiding this comment

Luka-sama commented Sep 2, 2024

skjnldsv commented Sep 3, 2024

welcome bot commented Sep 3, 2024

Pytal commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 6, 2024