-
Notifications
You must be signed in to change notification settings - Fork 19
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 2 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 |
|---|---|---|
|
|
@@ -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 |
|---|---|---|
|
|
@@ -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(): | ||
| 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: | ||
| 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.