Skip to content
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

Adding name col to CrontabSchedule model #478

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

trianglesis
Copy link

Having a lot of Cron time schedule items is frustrating when they have no verbose and short names, for example.

image

I'm trying to add a name to each - so it will be more visible, what task run on which time, actually.

image
image

With this change, it becomes more visible for the human eye what schedule is used for each task.
It's also better visible when you want to switch schedules, but the dropdown list is too big.

image

In addition: for years of usage - I have collected dozens of unused schedules and it starting to become a nightmare when you want to clean it up.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also pull from master please

@@ -0,0 +1,18 @@
# Generated by Django 3.1.7 on 2021-12-15 13:06
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use django 3.2.x to regenerate this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-created with 3.2

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trianglesis It doesn't seem necessary to do the if twice. I'll checkout your branch in my projects.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

…y Django 3.2

     (venv) PS D:\Projects\..\django-celery-beat> python -m django --version
      3.2
@auvipy
Copy link
Member

auvipy commented Dec 17, 2021

@lvelvee kindly have a review when have time


name = None
if self.name:
name = '[{}]'.format(self.name)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I wanted to achieve [Name in brackets], but leave an empty '' if there is no name.
Making if and if - makes it more visible on code, but It could be changed back to 1st version.
Thanks!

@@ -299,6 +299,13 @@ class CrontabSchedule(models.Model):
'Timezone to Run the Cron Schedule on. Default is UTC.'),
)

name = models.CharField(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would also like to have unit tests for new proposed changes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't run any test from test_schedulers.py with the current setup.
Will try to figure out what am I doing wrong, and possibly add some tests.

@auvipy auvipy added this to the v2.3.0 milestone Jan 3, 2022
~ list_display: added name of the "Periodic Task" rather than str: it made the name field easy to read and sort by.
~ list_filter: added "crontab" sequence to the very end, it helps to sort all "Periodic Task"s related to selected "crontab".
~ Arguments: not collapsed, usually when you're editing a task - you edit crontab or arguments. No need to click each time to open collapsed block.
models.py
~ CrontabSchedule: simpler name for crontab
~ PeriodicTask: do not show interval or crontab in the name of the task, since we have a table col for that already. It also enabled sort by this name.
@@ -116,9 +116,9 @@ class PeriodicTaskAdmin(admin.ModelAdmin):
model = PeriodicTask
celery_app = current_app
date_hierarchy = 'start_time'
list_display = ('__str__', 'enabled', 'interval', 'start_time',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why str is removed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using str there is no possible way to sort\group tasks by name.
Name is an essential feature that can be used to make sort easy and cron tasks more meaningful.

image

In this example, I can add prefixes in the name, and be able to group all tasks by any logical human-understandable meaning.

In addition, I think there is no need to add extra fields in the name when we have separate Crontab, Interval columns in the table already.

~ list_display: added name of the "Periodic Task" rather than str: it made the name field easy to read and sort by.
~ list_filter: added "crontab" sequence to the very end, it helps to sort all "Periodic Task"s related to selected "crontab".
~ Arguments: not collapsed, usually when you're editing a task - you edit crontab or arguments. No need to click each time to open collapsed block.
models.py
~ CrontabSchedule: simpler name for crontab
~ PeriodicTask: do not show interval or crontab in the name of the task, since we have a table col for that already. It also enabled sort by this name.
admin.py
~ Added view form for CrontabSchedule, sorting by name, display name, __str__, timezone, made easy to sort\group items, rename, fix or delete obsolete ones.
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unit tests for the changes to avoid possible regression?

@trianglesis
Copy link
Author

I'll try to add tests as soon as I figure out how to execute them all. Unfortunately, I can't get to work few tests related to model testing, which I wanted to use as a draft\example to test my changes.

Oleksandr Danylchenko added 2 commits January 25, 2022 10:47
~ list_display: added name of the "Periodic Task" rather than str: it made the name field easy to read and sort by.
~ list_filter: added "crontab" sequence to the very end, it helps to sort all "Periodic Task"s related to selected "crontab".
~ Arguments: not collapsed, usually when you're editing a task - you edit crontab or arguments. No need to click each time to open collapsed block.
models.py
~ CrontabSchedule: simpler name for crontab
~ PeriodicTask: do not show interval or crontab in the name of the task, since we have a table col for that already. It also enabled sort by this name.
admin.py
~ Added view form for CrontabSchedule, sorting by name, display name, __str__, timezone, made easy to sort\group items, rename, fix or delete obsolete ones.
unit test_models.py
+ Add simple test on creation named crontab entry with possible valid names.
~ list_display: added name of the "Periodic Task" rather than str: it made the name field easy to read and sort by.
~ list_filter: added "crontab" sequence to the very end, it helps to sort all "Periodic Task"s related to selected "crontab".
~ Arguments: not collapsed, usually when you're editing a task - you edit crontab or arguments. No need to click each time to open collapsed block.
models.py
~ CrontabSchedule: simpler name for crontab
~ PeriodicTask: do not show interval or crontab in the name of the task, since we have a table col for that already. It also enabled sort by this name.
admin.py
~ Added view form for CrontabSchedule, sorting by name, display name, __str__, timezone, made easy to sort\group items, rename, fix or delete obsolete ones.
django_celery_beat models.py
~ There was an extra space on __str__ when name is not set. Now it shouldn't appear.
unit test_models.py
+ Add simple test on creation named crontab entry with possible valid names.
unit test_schedulers.py
+ Assert PeriodicTask name is in correct format and have a space and [] braces.
~ Assert PeriodicTask name without cron\interval appendix.
+ Assert PeriodicTask cron\interval __str__ is expected in human-readable format.
@trianglesis
Copy link
Author

Currently

  • testing the creation of Crontab with valid names or an empty string.
  • testing that PeriodicTask.name has no extra values in it and if PeriodicTask.interval and PeriodicTask.crontab are in a human-readable format (as it was before in name).

@trianglesis trianglesis requested a review from auvipy January 27, 2022 17:38
@auvipy auvipy removed this from the v2.3.0 milestone Oct 13, 2022
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please rebase the conflicts please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants