-
Notifications
You must be signed in to change notification settings - Fork 20
Sync Media with MEDIA_PLUGINS setting robustly #1830
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| Alter how the `MEDIA_PLUGINS` setting is synced with the Media model. The goal | ||
| is to be able to eventually easily replace the plugin used for a specific media | ||
| type with another. This handles the removal of plugins once installed much | ||
| better. | ||
|
|
||
| NOTES: | ||
|
|
||
| Added a new management command `sync_media` that should be run after `migrate` | ||
| but before running up the actual server in order to ensure that the `Media` | ||
| table is up to date with installed media plugins even if a plugin has been | ||
| removed since first startup. Destinations using removed plugins will still | ||
| exist, they just won't work. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,16 +11,16 @@ def ready(self): | |
| from .signals import ( | ||
| create_default_timeslot, | ||
| sync_email_destination, | ||
| sync_media, | ||
| task_background_send_notification, | ||
| trigger_sync_media, | ||
| ) | ||
|
|
||
| # uses settings | ||
| from .utils import are_notifications_enabled | ||
|
|
||
| post_save.connect(create_default_timeslot, "argus_auth.User") | ||
| post_save.connect(sync_email_destination, "argus_auth.User") | ||
| post_migrate.connect(sync_media, sender=self) | ||
| post_migrate.connect(trigger_sync_media, sender=self) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But this means it is only synced after migrations? The settings can be changed without running migrations
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is needed for the tests to work. It's either this or running sync_media whenever PersonUserFactory is used, or turn off the signal that creates the default destination from email address. The idea is to run the management command, like Follow-up: we should probably have a command "sync" that collects all the stuff we want to do before running up the actual server in prod:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, then we should document that really well, that this needs to be run any time the media settings are changed |
||
|
|
||
| if are_notifications_enabled(): | ||
| # sending notifications | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import sys | ||
|
|
||
| from django.core.management.base import BaseCommand | ||
|
|
||
| from argus.notificationprofile.media import sync_media | ||
|
|
||
|
|
||
| class Command(BaseCommand): | ||
| help = "Sync anything needing syncing that is not covered by migrate" | ||
|
|
||
| def add_arguments(self, parser): | ||
| parser.add_argument("-l", "--list", action="store_true", help="Show the available syncs that will be run") | ||
|
|
||
| def handle(self, *args, **options): | ||
| if options.get("list", False): | ||
| sys.stdout.write("Sync MEDIA_PLUGINS setting to MEDIA table") | ||
| sys.exit(0) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit/Q: Would a simple return not suffice here instead of forcibly exiting?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I simplified it instead. |
||
|
|
||
| sync_media() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ | |
|
|
||
|
|
||
| __all__ = [ | ||
| "sync_media", | ||
| "api_safely_get_medium_object", | ||
| "send_notification", | ||
| "background_send_notification", | ||
|
|
@@ -41,18 +42,65 @@ | |
|
|
||
|
|
||
| # TODO: Raise Incident if media_class not importable? | ||
|
|
||
| MEDIA_PLUGINS = getattr(settings, "MEDIA_PLUGINS") | ||
| _media_classes = [import_class_from_dotted_path(media_plugin) for media_plugin in MEDIA_PLUGINS] | ||
| MEDIA_CLASSES_DICT = {media_class.MEDIA_SLUG: media_class for media_class in _media_classes} | ||
|
|
||
|
|
||
| def _compile_MEDIA_CLASSES_DICT(): | ||
|
Check warning on line 49 in src/argus/notificationprofile/media/__init__.py
|
||
| media_classes_dict = {} | ||
| for media_plugin in MEDIA_PLUGINS: | ||
| try: | ||
| plugin_class = import_class_from_dotted_path(media_plugin) | ||
| except (ImportError, ModuleNotFoundError, AttributeError) as e: | ||
|
Check warning on line 54 in src/argus/notificationprofile/media/__init__.py
|
||
| LOG.warning('Failed to load media plugin "%s": %s', media_plugin, str(e)) | ||
| else: | ||
| media_classes_dict[plugin_class.MEDIA_SLUG] = plugin_class | ||
| return media_classes_dict | ||
|
|
||
|
|
||
| MEDIA_CLASSES_DICT = _compile_MEDIA_CLASSES_DICT() | ||
|
|
||
|
|
||
| def sync_media(): | ||
| """Sync MEDIA_PLUGINS with the Media table | ||
|
|
||
| Check if all media in Media has a respective class""" | ||
|
|
||
| for medium in Media.objects.all(): | ||
| is_installed = medium.slug in MEDIA_CLASSES_DICT.keys() | ||
| if is_installed != medium.installed: | ||
| medium.installed = is_installed | ||
| medium.save(update_fields=["installed"]) | ||
| if not is_installed: | ||
| LOG.warning("%s plugin is no longer registered in MEDIA_PLUGINS", medium.name) | ||
| else: | ||
| LOG.info("%s plugin is again registered in MEDIA_PLUGINS", medium.name) | ||
|
|
||
| # Check if all media plugins are also saved in Media | ||
| for media_class in MEDIA_CLASSES_DICT.values(): | ||
| if not Media.objects.filter(slug=media_class.MEDIA_SLUG): | ||
|
hmpf marked this conversation as resolved.
|
||
| media = Media( | ||
| slug=media_class.MEDIA_SLUG, | ||
| name=media_class.MEDIA_NAME, | ||
| installed=True, | ||
| ) | ||
| media.save() | ||
| LOG.info("%s media plugin is now installed and registered correctly", media.name) | ||
|
|
||
|
|
||
| def api_safely_get_medium_object(media_slug, version: str = API_STABLE_VERSION): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function changed from throwing an exception (which the system can handle) to just returning
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tech debt, not updated for supporting both API and HTMx. Will fix.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like we'll have to postpone this PR, I don't think I'll have the time to finish it this sprint: The problem is the remaining intermixing of API and non API in the destinations CRUD handling. The purpose of this PR was to not crash out hard in the HTMx frontend when the MEDIA_PLUGINS setting was out of sync with the rows in the Media table, which was fulfilled. Making "api_safely_get_medium_object" better would need yet another refactor, so I need to down-prioritize this. |
||
| try: | ||
| classobj = MEDIA_CLASSES_DICT[media_slug] | ||
| except KeyError: | ||
| medium_obj = Media.objects.get(slug=media_slug) | ||
| except Media.DoesNotExist: | ||
| raise ValidationError(f'Medium "{media_slug}" is not installed.') | ||
| obj = classobj(version) | ||
| return obj | ||
| if medium_obj.installed: | ||
| try: | ||
| classobj = MEDIA_CLASSES_DICT[media_slug] | ||
| except KeyError: | ||
| raise ValidationError(f'Python module for "{media_slug}" not found') | ||
| obj = classobj(version) | ||
| return obj | ||
| return None | ||
|
|
||
|
|
||
| def send_notification(destinations: Iterable[DestinationConfig], *events: Iterable[Event]): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,12 +133,9 @@ def get_context_data(self, **kwargs): | |
| class MediaViewSet(viewsets.ModelViewSet): | ||
| version = VERSION | ||
| serializer_class = MediaSerializer | ||
| queryset = Media.objects.none() | ||
| queryset = Media.objects.available() | ||
| http_method_names = ["get", "head"] | ||
|
|
||
| def get_queryset(self): | ||
| return Media.objects.all() | ||
|
|
||
| @extend_schema(responses={"200": JSONSchemaSerializer}) | ||
| @action(methods=["get"], detail=True) | ||
| def json_schema(self, request, pk, *args, **kwargs): | ||
|
|
@@ -178,7 +175,7 @@ class DestinationConfigViewSet(rw_viewsets.ModelViewSet): | |
| http_method_names = ["get", "head", "post", "patch", "delete"] | ||
|
|
||
| def get_queryset(self): | ||
| return self.request.user.destinations.all() | ||
| return self.request.user.destinations.available() | ||
|
|
||
| def perform_create(self, serializer): | ||
| serializer.validated_data["settings"].pop("synced", False) | ||
|
|
@@ -188,6 +185,9 @@ def destroy(self, request, *args, **kwargs): | |
| pk = self.kwargs["pk"] | ||
| destination = get_object_or_404(self.get_queryset(), pk=pk) | ||
|
|
||
| medium = api_safely_get_medium_object(destination.media.slug) | ||
| if not medium: | ||
| raise ValidationError(f"Media {destination.media.slug} of destination not installed") | ||
| try: | ||
| medium = api_safely_get_medium_object(destination.media.slug, self.version) | ||
|
Comment on lines
+188
to
192
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, |
||
| medium.raise_if_not_deletable(destination) | ||
|
|
@@ -197,8 +197,10 @@ def destroy(self, request, *args, **kwargs): | |
| return super().destroy(destination) | ||
|
|
||
| def _is_destination_duplicate(self, destination): | ||
| other_destinations = DestinationConfig.objects.filter(media=destination.media).filter( | ||
| ~Q(user_id=destination.user.id) | ||
| other_destinations = ( | ||
| DestinationConfig.objects.available() | ||
| .filter(media=destination.media) | ||
| .filter(~Q(user_id=destination.user.id)) | ||
| ) | ||
| medium = api_safely_get_medium_object(destination.media_id, self.version) | ||
| destination_in_use = medium.has_duplicate(other_destinations, destination.settings) | ||
|
|
@@ -213,6 +215,8 @@ def duplicate(self, request, pk, *args, **kwargs): | |
| destination = request.user.destinations.get(pk=pk) | ||
| except DestinationConfig.DoesNotExist: | ||
| raise ValidationError(f"Destination with pk={pk} does not exist.") | ||
| if not destination.media.installed: | ||
| raise ValidationError(f"Destination with pk={pk} is not supported, plugin not installed correctly.") | ||
| is_duplicate = self._is_destination_duplicate(destination=destination) | ||
| serializer = DuplicateDestinationSerializer({"is_duplicate": is_duplicate}) | ||
| return Response(serializer.data) | ||
|
|
||
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.
Nit: Hmm, this doesn't seem to give sufficient feedback to the user if the delete doesn't happen but they get a success response, it's probably confusing.