Skip to content

Conversation

john-parton
Copy link
Contributor

@john-parton john-parton commented Jul 24, 2024

Fixed #35628, allow GeneratedField with appropriate output_field for date_hierarchy.

Trac ticket number

This is the basic fix for https://code.djangoproject.com/ticket/35628# , but without tests/docs. Will add tests/docs if approach is deemed appropriate.

Branch description

Essentially, there's a check to make sure that date_hierarchy is a DateField or DateTimeField, but there is no technical reason to prohibit a GeneratedField with output_field set to DateField or DateTimeField.

This adjusts the check to allow that specific case through.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

@john-parton john-parton force-pushed the bugfix/35628-generate-field-date-heirarchy branch from 136ab4d to bb7b63e Compare July 24, 2024 18:58
@john-parton john-parton changed the title Allow for 'GeneratedField' without appropriate output_field for date_… Fix ticket #35628, allow GeneratedField with appropriate output_field for date_hierarchy. Jul 24, 2024
@john-parton john-parton force-pushed the bugfix/35628-generate-field-date-heirarchy branch from bb7b63e to 9e8083e Compare July 24, 2024 19:01
@john-parton john-parton changed the title Fix ticket #35628, allow GeneratedField with appropriate output_field for date_hierarchy. Fixed #35628, allow GeneratedField with appropriate output_field for date_hierarchy. Jul 24, 2024
@john-parton
Copy link
Contributor Author

john-parton commented Jul 24, 2024

As an alternate implementation, we could do something like

if field.get_internal_type() in {"DateField", "DateTimeField"}:
    ...

This would also permit user defined date fields that don't explicitly subclass DateField or DateTimeField so long as the user defines the get_internal_type method.

However, I tried to keep the structure of the code roughly the same.

@john-parton john-parton force-pushed the bugfix/35628-generate-field-date-heirarchy branch 2 times, most recently from b3c9c4a to 5820aec Compare August 2, 2024 02:46
@john-parton
Copy link
Contributor Author

I added a release note.

Is there a way that I can render out those pages locally so that I can make sure they're formatted nicely?

I don't think it's necessary to add a 'version changed' note to the date_hierarchy docs. I think a typical user of the GeneratedField will just assume it's going to work.

@sarahboyce
Copy link
Contributor

I added a release note.

Thank you! This is a special case where the release note belongs in the 5.0.8 release notes (as this is a release blocker for a monthly release of 5.0). Something like

* Fixed a bug in Django 5.0 where a system check would crash when
  ``ModelAdmin.date_hierarchy`` was set to a ``GeneratedField`` with an
  ``output_field`` of a ``DateField`` or a ``DateTimeField``
  (:ticket:`35628`).

You might need to rebase with the latest main

Is there a way that I can render out those pages locally so that I can make sure they're formatted nicely?

Yes, see this guide also have the requirements of docs/requirements.txt installed 👍

I don't think it's necessary to add a 'version changed' note to the date_hierarchy docs. I think a typical user of the GeneratedField will just assume it's going to work.

Correct 👍

@john-parton john-parton force-pushed the bugfix/35628-generate-field-date-heirarchy branch 2 times, most recently from 7642850 to aa97af5 Compare August 2, 2024 15:46
@john-parton
Copy link
Contributor Author

  • Rewrote the tests using the assertion pattern discussed previously
  • Moved release note to 5.0.8 bugfixes section
  • Squashed my commit and cherry-picked it against main

@john-parton john-parton force-pushed the bugfix/35628-generate-field-date-heirarchy branch 2 times, most recently from 029b33c to a7cf927 Compare August 2, 2024 15:50
@john-parton
Copy link
Contributor Author

Fixed linter errors, squashed, force-pushed.

@sarahboyce sarahboyce force-pushed the bugfix/35628-generate-field-date-heirarchy branch from a7cf927 to eeb172e Compare August 3, 2024 07:29
@sarahboyce sarahboyce changed the title Fixed #35628, allow GeneratedField with appropriate output_field for date_hierarchy. Fixed #35628 -- Allowed compatible GeneratedFields for ModelAdmin.date_hierarchy. Aug 3, 2024
@sarahboyce sarahboyce force-pushed the bugfix/35628-generate-field-date-heirarchy branch 2 times, most recently from d4e7d12 to 4c46ee1 Compare August 3, 2024 07:37
Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you @john-parton ⭐ this looks good to me

@sarahboyce sarahboyce requested a review from charettes August 3, 2024 07:38
@pauloxnet
Copy link
Member

LGTM

@sarahboyce sarahboyce force-pushed the bugfix/35628-generate-field-date-heirarchy branch from 4c46ee1 to 6c368bf Compare August 5, 2024 13:15
@sarahboyce sarahboyce merged commit 7f8d839 into django:main Aug 5, 2024
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.

4 participants