[ ENG-9762] Improve moderator and user files tests to properly reflect digest code#254
[ ENG-9762] Improve moderator and user files tests to properly reflect digest code#254Johnetordoff wants to merge 8 commits into
Conversation
…e also change listener code to reflect this
Johnetordoff
changed the title
| if provider is None: | ||
| log_message(f"subscribed_object fpr {subscribed_object} does not exist") | ||
| log_message(f"provider for subscribed_object {subscribed_object} does not exist") | ||
| email_task.status = 'PROVIDER NOT FOUND' |
There was a problem hiding this comment.
The email_task.status is invalid in lines 122, 129, 136
| log_message(f"subscribed_object fpr {subscribed_object} does not exist") | ||
| log_message(f"provider for subscribed_object {subscribed_object} does not exist") | ||
| email_task.status = 'PROVIDER NOT FOUND' | ||
| email_task.save() |
There was a problem hiding this comment.
It looks like this is the place for the email_task.error_message
| NotificationType.Type.PROVIDER_NEW_PENDING_WITHDRAW_REQUESTS.instance.emit( | ||
| user=recipient, | ||
| subscribed_object=provider, | ||
| subscribed_object=resource, |
There was a problem hiding this comment.
I have some concerns about changing "subscribed_object" to "resource" instead of "provider."
Currently, with "subscribed_object" set to "provider," there is one subscription for each user-provider pair. However, using "resource" for "subscribed_object" would result in a subscription for each user-resource pair. This would create a large number of records in the database that would be difficult to work with.
Here's an example:
One provider
10 moderators
100 resources.
Using the user-provider pair would result in 10 records.
However, if we use the user-resource pair, we will have 1,000 records. If a user updates the frequency, 100 records must be updated.
There was a problem hiding this comment.
I agree and I don't see any benefits for the change. Let's keep using user-provider pair.
| NotificationType.Type.PROVIDER_NEW_PENDING_WITHDRAW_REQUESTS.instance.emit( | ||
| user=recipient, | ||
| subscribed_object=provider, | ||
| subscribed_object=resource, |
There was a problem hiding this comment.
I agree and I don't see any benefits for the change. Let's keep using user-provider pair.
| 'schedule': crontab(minute='*/10'), # Daily 12 a.m (ten minutes for testing purposes) | ||
| 'kwargs': {'dry_run': False}, | ||
| }, | ||
| 'send_users_digest_email': { | ||
| 'task': 'notifications.tasks.send_users_digest_email', | ||
| 'schedule': crontab(minute=0, hour=5), # Daily 12 a.m | ||
| 'schedule': crontab(minute='*/10'), # Daily 12 a.m (ten minutes for testing purposes) |
There was a problem hiding this comment.
Don't change defaults, these needs to be changed in each server's local.py
| 'task': 'scripts.triggered_mails', | ||
| 'schedule': crontab(minute=0, hour=5), # Daily 12 a.m | ||
| 'kwargs': {'dry_run': False}, | ||
| 'kwargs': {'dry_run': True}, # For no_login messages |
There was a problem hiding this comment.
This is good. We should have True for defaults and set it accordingly in each server's local.py.
3 participants
Make changes to help working of digest logic and tests.
Purpose
Changes
QA Notes
Please make verification statements inspired by your code and what your code touches.
What are the areas of risk?
Any concerns/considerations/questions that development raised?
Documentation
Side Effects
Ticket
https://openscience.atlassian.net/browse/ENG-9762