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

Remove unnecessary code in created method. #438

Merged
merged 4 commits into from
Oct 5, 2022

Conversation

2ykwang
Copy link
Member

@2ykwang 2ykwang commented Oct 4, 2022

related #404

already run the same code in the Django
ref: https://github.com/django/django/blob/3.2/django/contrib/admin/utils.py#L381-L429

# ...
elif isinstance(field, models.DateTimeField):
    return formats.localize(timezone.template_localtime(value))
# ...

@2ykwang 2ykwang self-assigned this Oct 4, 2022
@2ykwang 2ykwang marked this pull request as draft October 4, 2022 09:44
@aleh-rymasheuski
Copy link
Contributor

@2ykwang, the main part of the created method is a call to localtime. It matters when the server time is not UTC (it's set to NY time in my case).

These are the outputs of your version, the current version, and a compromise version which delegates the formatting to Django code:

    def created(self, obj):
        return obj.timestamp

Created: 2022-10-03 16:09:59.723996+00:00

    def created(self, obj):
        return localtime(obj.timestamp).strftime("%Y-%m-%d %H:%M:%S")

Created: 2022-10-03 12:09:59

    def created(self, obj):
        return localtime(obj.timestamp)

Created: 2022-10-03 12:09:59.723996-04:00

I would argue that precision to seconds and implicit server-local timezone are beneficial to the readability.

@2ykwang
Copy link
Member Author

2ykwang commented Oct 4, 2022

@alieh-rymasheuski When the created method returns a DateTimeField object, it displays the data formatted according to Django settings. in my opinion, it felt awkward because the LogEntryMixin.created date format was different from other Django Admin date formats.

def created(self, obj):
    return localtime(obj.timestamp).strftime("%Y-%m-%d %H:%M:%S")

# output: created - 2022-09-12 23:33:09
def created(self, obj):
    return obj.timestamp

# output: created - 2022년 9월 12일 11:33 오후
# display timestamp field instead of the created method.
class LogEntryAdmin(admin.ModelAdmin, LogEntryAdminMixin):
    # ... 
    list_display = ["created", "resource_url", "action", "msg_short", "user_url"]
    # ...

# output: timestamp - 2022년 9월 12일 11:33 오후

I think it's natural to match the DateTimeField format shown by other admins.

@aleh-rymasheuski
Copy link
Contributor

Agree. But please make sure to keep the localtime call in place - it is valuable for the case when the server timezone is different from UTC.

@2ykwang 2ykwang marked this pull request as ready for review October 4, 2022 14:29
@codecov
Copy link

codecov bot commented Oct 4, 2022

Codecov Report

Merging #438 (5057014) into master (993cd84) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 5057014 differs from pull request most recent head a6cdbe9. Consider uploading reports for the commit a6cdbe9 to get more accurate results

@@            Coverage Diff             @@
##           master     #438      +/-   ##
==========================================
+ Coverage   92.29%   92.30%   +0.01%     
==========================================
  Files          24       24              
  Lines         727      728       +1     
==========================================
+ Hits          671      672       +1     
  Misses         56       56              
Impacted Files Coverage Δ
auditlog/mixins.py 87.09% <100.00%> (+0.14%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@2ykwang
Copy link
Member Author

2ykwang commented Oct 4, 2022

@alieh-rymasheuski
thank you I just edited the code.

@aleh-rymasheuski
Copy link
Contributor

@2ykwang, please add a changelog entry as well.

@hramezani, would you like to take a look too?

@2ykwang
Copy link
Member Author

2ykwang commented Oct 4, 2022

@alieh-rymasheuski done!

@@ -16,11 +17,11 @@


class LogEntryAdminMixin:
@admin.display(description="created")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@admin.display(description="created")
@admin.display(description="Created")

Use the `display` decorator instead of `short_description`
CHANGELOG.md Outdated
@@ -7,6 +7,7 @@

#### Fixes

- fix: Remove unnecessary code in `created` method. ([#438](https://github.com/jazzband/django-auditlog/pull/438))
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need to mention this here as it's not adding/changing a feature.
Please remove it.

@2ykwang 2ykwang requested a review from hramezani October 5, 2022 16:37
@hramezani hramezani merged commit 487b8ab into jazzband:master Oct 5, 2022
@hramezani hramezani deleted the aware-created branch October 5, 2022 16:39
@hramezani
Copy link
Member

hramezani commented Oct 5, 2022

Thanks @2ykwang for the patch and @alieh-rymasheuski for reviewing!

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