-
Notifications
You must be signed in to change notification settings - Fork 441
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
Publish annotation tasks to an authority queue #9402
Conversation
task_name: str | ||
|
||
|
||
class AnnotationAuthorityQueueService: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we wouldn't need a separate service for this, but as this is going to be only used by LMS for now it isolates it from the rest of the codebase.
|
||
|
||
@subscriber(AnnotationEvent) | ||
def publish_annotation_event_for_authority(event): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, we do another level of indirection here, the subscriber creates a celery task, that way if this LMS only feature creates any problems the annotations will be created fine.
@celery.task | ||
def publish_annotation_event_for_authority(event_action, annotation_id): | ||
"""Optionally publish an annotation event to the authority's message queue.""" | ||
celery.request.find_service(AnnotationAuthorityQueueService).publish( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The celery task just calls the service method, not much to see here.
if not authority_queue_config: | ||
return | ||
|
||
if event_action != "create" or not annotation.mentions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This limits the amount of messages created by this to... just us testing for now.
self, config_json: str | None | ||
) -> dict[str, AuthorityQueueConfiguration]: | ||
"""Parse the authority queue config JSON string into dictionary by authority name.""" | ||
if not config_json: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being careful, if no config, broken config or any other issue, we just log and continue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only checked once on the server instantiation.
374d257
to
d040991
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Based on a per authority configuration publish new annotation messages that containt mentions to a queue. The motivation for this is to support mention notification for LMS users.
d040991
to
ee713cc
Compare
Based on a per authority configuration publish new annotation messages that contain mentions to a queue.
The motivation for this is to support mention notification for LMS users.
See the design document over: https://docs.google.com/document/d/1tOTw-9bqCgM1bp0h2QsjsGKl0mtGh2lnpdv5oHYIj8E
Testing
https://github.com/hypothesis/devdata/pull/116/files#diff-fe2be50e5855174f3ad7a3348f4381308c4b70bacd6eb95fa47342f448ad1728R3
Checkout this LMS branch Consume messages about annotations published by H lms#7051
Restart H's and LMS's
make dev
so celery changes are picked upMake an annotation on a assignment
https://hypothesis.instructure.com/courses/125/assignments/873