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

fix: do not send daily digest email to user who is disabled #1803

Merged
merged 7 commits into from
Oct 28, 2024

Conversation

yemkareems
Copy link
Contributor

Add a extra check to not send the email to user who is disabled.

In my local i made jane a disabled user

image

And add the condition. This triggered a email to alice, bob and john. And it did not send the email to jane

image

Copy link

cypress bot commented Sep 24, 2024

Activity    Run #2059

Run Properties:  status check passed Passed #2059  •  git commit 8789e1bf33: fix: do not send daily digest email to user who is disabled
Project Activity
Branch Review fix/834/dont-sent-digest-email-to-disabled-user
Run status status check passed Passed #2059
Run duration 02m 56s
Commit git commit 8789e1bf33: fix: do not send daily digest email to user who is disabled
Committer yemkareems
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 1
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 10
View all changes introduced in this branch ↗︎

Copy link
Collaborator

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Seems enough 👍

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Would it be possible to instead take an IUser object in sendDigestForUser and check the enabled state in parent method sendDigests ?
That would allow to also not update the activity.digest config value for disabled users, as that may be confusing. Would also be a bit better in performance to bail out earlier for disabled users.

@nickvergessen
Copy link
Member

That would allow to also not update the activity.digest config value for disabled users, as that may be confusing. Would also be a bit better in performance to bail out earlier for disabled users.

I was wondering if we should even call

$this->config->setUserValue($user->getUID(), 'activity', 'activity_digest_last_send', (string)$lastActivityId);

so after re-enabling the user is not spamed with old legacy info from the day they got disabled.

@yemkareems
Copy link
Contributor Author

That would allow to also not update the activity.digest config value for disabled users, as that may be confusing. Would also be a bit better in performance to bail out earlier for disabled users.

I was wondering if we should even call

$this->config->setUserValue($user->getUID(), 'activity', 'activity_digest_last_send', (string)$lastActivityId);

so after re-enabling the user is not spamed with old legacy info from the day they got disabled.

Hi @nickvergessen , @come-nc Do we need to do this for disabled user. I see there is a ACTIVITY_LIMIT and first time after re enabling user the notifications ll be cluttered and after that it should be fine. Please let me know and i ll do it accordingly

@yemkareems
Copy link
Contributor Author

yemkareems commented Sep 25, 2024

Hi @come-nc , @nickvergessen I will add the user disabled check in sendDigests method as Côme suggested but will also add one updateLastSentForUser which will do
$this->config->setUserValue($user->getUID(), 'activity', 'activity_digest_last_send', (string)$lastActivityId);
as Joas suggested.

@yemkareems
Copy link
Contributor Author

yemkareems commented Sep 26, 2024

Hi @come-nc, @nickvergessen I have done this change as requested but however i do not see a entry in oc_preferences for the disabled user(jane). I see the mail has gone out for admin user and only entry for admin user and not for the disabled user jane for whom i did some file rename just before disabling. Checking it locally now what is going wrong

image

@nickvergessen
Copy link
Member

Could you push, so we can see what your code is?

lib/DigestSender.php Outdated Show resolved Hide resolved
@yemkareems yemkareems requested a review from miaulalala October 15, 2024 04:50
@yemkareems yemkareems requested a review from come-nc October 17, 2024 08:32
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Good apart from 2 small changes.

lib/DigestSender.php Outdated Show resolved Hide resolved
lib/DigestSender.php Outdated Show resolved Hide resolved
… is not used

Signed-off-by: yemkareems <yemkareems@gmail.com>
@yemkareems
Copy link
Contributor Author

/backport to stable28

@yemkareems
Copy link
Contributor Author

/backport to stable29

@yemkareems
Copy link
Contributor Author

/backport to stable30

@artonge
Copy link
Collaborator

artonge commented Oct 28, 2024

/backport to stable28

@yemkareems
Copy link
Contributor Author

/backport to stable29

1 similar comment
@artonge
Copy link
Collaborator

artonge commented Oct 28, 2024

/backport to stable29

@artonge
Copy link
Collaborator

artonge commented Oct 28, 2024

/backport to stable30

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.

6 participants