Adding human readable descriptions of crontab schedules #622

truthdoug · 2022-12-22T19:43:50Z

Partially addresses #612

What I did

incorporated cron-descriptor to provide more readable descriptions of crontab schedules
added these descriptions on the PeriodicTask detail view on and the Crontab list view
updated help text for Crontab to help me remember how the numbers translate to days of the week
modest updates to locales

What is missing

testing of non-English locales
translation of new help text for locales other than English, Spanish, and German

Possible problems

The human readable text of all crontabs is loaded into the context of the PeriodicTask detail view. This could be a problem for sites that have a very large number of crontab schedules.

django_celery_beat/models.py

auvipy · 2023-01-13T05:51:39Z

django_celery_beat/admin.py

+    def changeform_view(self, request, object_id=None, form_url='',
+                        extra_context=None):
+        extra_context = extra_context or {}
+        crontabs = CrontabSchedule.objects.all()


instead of calling all, can we reduce the number?

Hmmm, I'm not sure how we'd decide which to include. My goal here is to provide natural language translations for all available crontabs.

The best argument for reducing the number is possible slow response times when loading the page. But, based on other sites I've used that load a lot of data into javascript objects onto the DOM, my hunch is that there would have to be hundreds (if not thousands) of CrontabSchedule objects before it would impact performance of this page. Additionally, since this is an admin-only view, any hit to responsiveness would never be seen by end users.

Do you have thoughts on how we might choose which ones to reduce it to?

can we show 20/25 per page?

It's not clear to me how paginating this would work.

My goal with this change is to update the human readable description anytime the user changes the select list of defined crontabs. So, if I change from 5 * * * * to 0 10 * * 1, I want the description to change from "At 5 minutes past the hour" to "At 10:00 AM, only on Monday" to verify that the change is what I expect.

If the code only loads 20 per page, what happens if I choose a crontab that wasn't part of the 20 that were loaded?

This could be implemented as ajax/XHR so that the description is readable only on demand. Since django-celery-beat doesn't currently have any custom endpoints defined, this seems like a bigger change. Alternately, there could be a setting to disable this if a site has so many CrontabSchedules defined that loading this admin page is onerous.

Can you tell me more about your objection to having all defined on this page?

auvipy · 2023-01-13T05:55:25Z

django_celery_beat/templates/admin/djcelery/change_periodictask_form.html

+                `${originalCrontabHelp}<br/>Translation: ${additional}`);
+        };
+
+        django.jQuery(".field-crontab_translation").hide()


is this django.jQuery being used from django admin itself? is it possible to switch to plain vanilla JS here? ES 6 without any dependencies?

From django documentation:

To avoid conflicts with user-supplied scripts or libraries, Django’s jQuery (version 3.6.0) is namespaced as django.jQuery.

Since this namespaced jQuery comes with Django admin, this seems like the best choice for using jQuery. It's probably possible to use ES 6 w/o dependencies but jQuery is not an extra dependency, so I think using this is ok.

ok make sense

Updated all .po files with new help string. Added translations for es and de

for more information, see https://pre-commit.ci

auvipy · 2023-02-02T05:35:59Z

django_celery_beat/admin.py

+    def changeform_view(self, request, object_id=None, form_url='',
+                        extra_context=None):
+        extra_context = extra_context or {}
+        crontabs = CrontabSchedule.objects.all()


can we show 20/25 per page?

auvipy · 2023-02-07T05:16:54Z

I am not sure why CI is failing. can you check please?

khanh96le · 2023-02-07T07:42:33Z

django_celery_beat/models.py

@@ -616,7 +626,7 @@ def schedule(self):
        if self.interval:
            return self.interval.schedule
        if self.crontab:
-            return self.crontab.schedule
+            return self.crontab


Some unit tests are failed because of this change.
Can you elaborate more on why we need this?

I recently closed PR #621 because I realized the changes are already covered in yours.
Hope this can give you an idea.

I can merge part of your PR first @khanh96le

Sure @auvipy. I saw that you already reopened it

I like the change you made in #621, @khanh96le! That's an improved list view!

Sure @auvipy. I saw that you already reopened it

can you give this PR another round of review?

auvipy · 2023-02-07T10:49:36Z

builds are now passing after merging your PR @khanh96le

auvipy · 2023-03-02T10:08:59Z

The human readable text of all crontabs is loaded into the context of the PeriodicTask detail view. This could be a problem for sites that have a very large number of crontab schedules. --- we need to find a solution for this

auvipy self-requested a review December 23, 2022 07:14

auvipy requested changes Dec 23, 2022

View reviewed changes

django_celery_beat/models.py Show resolved Hide resolved

truthdoug force-pushed the issue612 branch 2 times, most recently from 2d096c6 to 8fef408 Compare December 23, 2022 23:04

auvipy requested changes Jan 13, 2023

View reviewed changes

dougharris and others added 4 commits February 1, 2023 16:02

Adding human-readable descriptions of crontab schedules

a9207f4

Adding helpful text to days of week.

4a2cbc9

Updated all .po files with new help string. Added translations for es and de

[pre-commit.ci] auto fixes from pre-commit.com hooks

beb471d

for more information, see https://pre-commit.ci

Updates to make flake8 happy

8ac4553

truthdoug force-pushed the issue612 branch from 8fef408 to 8ac4553 Compare February 1, 2023 21:03

auvipy requested changes Feb 2, 2023

View reviewed changes

auvipy mentioned this pull request Feb 6, 2023

Update PeriodicTaskAdmin list display #621

Merged

khanh96le reviewed Feb 7, 2023

View reviewed changes

Merge branch 'main' into issue612

70b424b

auvipy approved these changes Mar 2, 2023

View reviewed changes

auvipy merged commit 8f9fd1b into celery:main Mar 2, 2023

auvipy mentioned this pull request Mar 24, 2023

Exception in admin: invalid literal for int() with base 10: '1DAY' #647

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding human readable descriptions of crontab schedules #622

Adding human readable descriptions of crontab schedules #622

truthdoug commented Dec 22, 2022

auvipy Jan 13, 2023

truthdoug Jan 13, 2023

auvipy Feb 2, 2023

truthdoug Feb 2, 2023

auvipy Jan 13, 2023

truthdoug Jan 13, 2023

auvipy Jan 13, 2023

auvipy Feb 2, 2023

auvipy commented Feb 7, 2023

khanh96le Feb 7, 2023

khanh96le Feb 7, 2023

auvipy Feb 7, 2023

khanh96le Feb 7, 2023

truthdoug Feb 7, 2023

auvipy Feb 7, 2023

auvipy commented Feb 7, 2023

auvipy commented Mar 2, 2023

Adding human readable descriptions of crontab schedules #622

Adding human readable descriptions of crontab schedules #622

Conversation

truthdoug commented Dec 22, 2022

What I did

What is missing

Possible problems

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auvipy commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auvipy commented Feb 7, 2023

auvipy commented Mar 2, 2023