Skip to content

Conversation

@salmart-dev
Copy link
Contributor

@salmart-dev salmart-dev commented May 30, 2025

Summary

This PR fixes an issue where a user that has been inactive for a while, had their account disabled automatically by the background-job in this app and that has been re-enabled by an admin would get their account disabled again, if they would not log-in before the next run of the background-job.

Checklist

@salmart-dev salmart-dev self-assigned this May 30, 2025
@salmart-dev salmart-dev requested review from a team, artonge, icewind1991 and provokateurin and removed request for a team May 30, 2025 14:32
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

But otherwise looks good and solves the problem.
It didn't matter in the past as we only deleted users and disabling is rather new.

@salmart-dev
Copy link
Contributor Author

@nickvergessen I addressed all your comments and tested manually that it works as expected.

However, there seems to be an issue with the autoloader not finding the Test\TestCase class in CI, but locally tests run fine. Do you have any pointers that could help me troubleshoot this issue?

@nickvergessen
Copy link
Member

nickvergessen commented May 30, 2025

However, there seems to be an issue with the autoloader not finding the Test\TestCase class in CI, but locally tests run fine. Do you have any pointers that could help me troubleshoot this issue?

Basically needs a copy of nextcloud/notifications#2352
If it works locally for you, you might be running the tests "wrongly" (and/or with an old server master branch)

For apps that are not part of server, you need to go into the apps/user_retention/ directory and then install dependencies composer i and composer run test:unit


Edit: Merged that with #876
So rebasing should make tests work again

@salmart-dev salmart-dev force-pushed the fix/avoid-immediate-re-expiration branch from 4e8ec1f to ccbe684 Compare June 2, 2025 09:05
@salmart-dev
Copy link
Contributor Author

However, there seems to be an issue with the autoloader not finding the Test\TestCase class in CI, but locally tests run fine. Do you have any pointers that could help me troubleshoot this issue?

Basically needs a copy of nextcloud/notifications#2352 If it works locally for you, you might be running the tests "wrongly" (and/or with an old server master branch)

For apps that are not part of server, you need to go into the apps/user_retention/ directory and then install dependencies composer i and composer run test:unit

Edit: Merged that with #876 So rebasing should make tests work again

Hmm not sure to be honest.

Here's what I was doing:

  • Running Nextcloud with the nextcloud-docker-dev setup with the user_retention app in apps-extra
  • Install dependencies with docker compose exec -w /var/www/html/apps-extra/user_retention nextcloud composer install
  • Running docker compose exec -w /var/www/html/apps-extra/user_retention nextcloud composer test:unit

Tests were never failing because of autoload issues using this setup. But I do get some warnings from git for example, about the fact that the directory is owned by a user with id 1000 and not by the root user, but this should not be related.

In any case thank you for addressing the issue with the autoloader! I was planning to look into it first thing today but you were faster 🚀

@salmart-dev salmart-dev requested a review from nickvergessen June 3, 2025 08:13
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Having a look today or tomorrow

This commit fixes an issue where a user that has been inactive for a
while, had their account disabled automatically by this app and then
re-enabled by an admin would get their account disabled again, if they
would not log-in again before the next run of the background-job.

Signed-off-by: Salvatore Martire <[email protected]>
@nickvergessen nickvergessen force-pushed the fix/avoid-immediate-re-expiration branch from ccbe684 to 4b4203c Compare June 5, 2025 09:01
Signed-off-by: Joas Schilling <[email protected]>
@nickvergessen nickvergessen force-pushed the fix/avoid-immediate-re-expiration branch from 4b4203c to aed567e Compare June 5, 2025 09:46
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Cleaned up the coding style to my personal preference

@nickvergessen nickvergessen merged commit c1285fd into main Jun 5, 2025
41 checks passed
@nickvergessen nickvergessen deleted the fix/avoid-immediate-re-expiration branch June 5, 2025 09:54
@nickvergessen
Copy link
Member

/backport to stable29

@github-actions
Copy link
Contributor

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.)

@github-actions
Copy link
Contributor

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.)

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