Sync Media with MEDIA_PLUGINS setting robustly#1830
Conversation
Test results 6 files 1 182 suites 1m 55s ⏱️ Results for commit fba53ae. ♻️ This comment has been updated with latest results. |
hmpf
force-pushed
the
fix-sync-media
branch
3 times, most recently
from
0069cdc to
6a19a9b
Compare
|
johannaengland
force-pushed
the
fix-sync-media
branch
from
6a19a9b to
6e200af
Compare
johannaengland
left a comment
johannaengland
left a comment
There was a problem hiding this comment.
The tests are failing since you're never actually calling the sync_media function.
github-project-automation
Bot
moved this from 🏗 In progress
to ♻ Changes requested
in Argus development, public
|
I rebased the branch onto the newest main and fixed merge conflicts, so that I could review the PR, but I also noticed that the linting failed, which is why the tests weren't run. Linting failed due to the |
hmpf
force-pushed
the
fix-sync-media
branch
2 times, most recently
from
5eab970 to
47f37cd
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1830 +/- ##
==========================================
- Coverage 88.01% 87.77% -0.24%
==========================================
Files 139 140 +1
Lines 6916 6962 +46
==========================================
+ Hits 6087 6111 +24
- Misses 829 851 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hmpf
moved this from ♻ Changes requested
to ❓ Ready for review
in Argus development, public
|
| def handle(self, *args, **options): | ||
| if options.get("list", False): | ||
| sys.stdout.write("Sync MEDIA_PLUGINS setting to MEDIA table") | ||
| sys.exit(0) |
There was a problem hiding this comment.
Nit/Q: Would a simple return not suffice here instead of forcibly exiting?
There was a problem hiding this comment.
I simplified it instead.
| except ValidationError: | ||
| # should send 410 probably | ||
| return HttpResponseRedirect(self.get_success_url()) |
There was a problem hiding this comment.
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.
| 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): |
There was a problem hiding this comment.
This function changed from throwing an exception (which the system can handle) to just returning None for uninstalled media. Doesn't this add caller overhead? Maybe add a specific exception for "MediaNotInstalled".
Edit: This error is instead thrown in the API view, by raising a ValidationError. Would it not be simple to just raise it in api_safely_get_medium_object and omit the if not medium check in v2/views.py?
There was a problem hiding this comment.
Tech debt, not updated for supporting both API and HTMx. Will fix.
There was a problem hiding this comment.
api_safely_get_medium_object is called many, many places. This is becoming a rather expensive fix..
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
Hmm, api_safely_get_medium_object is assigned twice, the first instance only to check for existence, but then the medium is discarded and the db is queried again on line 192. Could you not just add self.version to the call on line 188? The version is only used in api_safely_get_medium_object if the media exists (passed to the classobj), so it makes essentially no difference to omit version here.
github-project-automation
Bot
moved this from ❓ Ready for review
to ♻ Changes requested
in Argus development, public
| 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) |
There was a problem hiding this comment.
But this means it is only synced after migrations? The settings can be changed without running migrations
There was a problem hiding this comment.
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 collectstatic, on every startup.
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:
- migrate (should be togglable)
- collectstatic
- sync_media
- .. whatever else we ever need to do in the future
There was a problem hiding this comment.
I see, then we should document that really well, that this needs to be run any time the media settings are changed
Fixed by adding back the post_migrate signal. |
3 participants
Scope and purpose
While testing #1803 it turned out the
argus_notificationprofile.Mediumtable was never synced properly with the MEDIA_PLUGINS setting.How to test manually
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to Argus can be found in the
Development docs.