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.

Sign up for GitHub

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

Jump to bottom

Properly handle empty file field #509

Merged
merged 1 commit into from
Apr 8, 2024
Merged

Properly handle empty file field #509

merged 1 commit into from
Apr 8, 2024

Conversation

PavelPancocha
Copy link
Contributor

  • Added some more code-coverage

Fix: #508

@PavelPancocha PavelPancocha mentioned this pull request Sep 11, 2023
Closed
@PavelPancocha
Copy link
Contributor Author

PavelPancocha commented Sep 11, 2023 •
edited
Loading

Local tests passed, the one was failing before:

====================================================================================================================== short test summary info =======================================================================================================================
FAILED tests/test_admin_views.py::test_detail - AssertionError: assert {'admin/base....html': 1, ...} == {'admin/base....html': 1, ...}
=================================================================================================================== 1 failed, 45 passed in 15.31s ====================================================================================================================

jamesgilmorelyst
jamesgilmorelyst reviewed Mar 23, 2024
View reviewed changes
Copy link

@jamesgilmorelyst jamesgilmorelyst left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Looks to me a good change once you fix the test.

tests/test_templatetags.py Outdated
"""
We can specify the name of a charfield or imagefield on our user model, or a callable that receives our user
"""
custom_jazzmin_settings["user_avatar"] = field
assert jazzmin.get_user_avatar(test_input) == expected
print(caplog.text)
Copy link

Choose a reason for hiding this comment

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

Can the print message be deleted, please?

Copy link
Owner

Choose a reason for hiding this comment

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

Once done there's no reason we can't get this in .

Copy link
Collaborator

Choose a reason for hiding this comment

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

@PavelPancocha You also need to rebase. I've QA'd this now and LGTM to be apart from that you still have dj_database_url in the test_app. So can you rebase and we can look into merging. Thanks for your help on this.

@jamesgilmorelyst jamesgilmorelyst added the bug Something isn't working label Mar 23, 2024
@PavelPancocha
Copy link
Contributor Author

@jamesgilmorelyst You took over the Jazzmin? Will you be able to merge it if it works?

@PavelPancocha
Copy link
Contributor Author

@jamesgilmorelyst @farridav please comment, that I know it is not a lost of time to revive this. Thanks!

PacificGilly
PacificGilly approved these changes Mar 26, 2024
View reviewed changes
Copy link
Collaborator

@PacificGilly PacificGilly left a comment

Choose a reason for hiding this comment

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

LGTM once rebased - comments are optional. Thanks.

tests/test_templatetags.py Outdated
"""
We can specify the name of a charfield or imagefield on our user model, or a callable that receives our user
"""
custom_jazzmin_settings["user_avatar"] = field
assert jazzmin.get_user_avatar(test_input) == expected
print(caplog.text)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@PavelPancocha You also need to rebase. I've QA'd this now and LGTM to be apart from that you still have dj_database_url in the test_app. So can you rebase and we can look into merging. Thanks for your help on this.

tests/test_templatetags.py
Comment on lines +92 to +95
if log:
assert log in caplog.text
else:
assert not caplog.text
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do love it when I see the logs being tested as well!

Optional - Whats your thoughts on testing log levels (i.e. WARNING here).

jazzmin/templatetags/jazzmin.py
if type(avatar_field) == str:
if avatar_field is not None:
if not avatar_field:
return no_avatar
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional - no_avatar is basically a constant so potentially could be moved outside the function to the top of the file (or to the settings).

@PavelPancocha
Copy link
Contributor Author

Thanks for feedback, great to see this project revived. I will find time to rebase/improve this PR during week!

@PavelPancocha PavelPancocha force-pushed the fix/warning_logger_when_imagefield_is_empty branch 2 times, most recently from 3044613 to f18cd80 Compare April 8, 2024 07:19
@PavelPancocha
Copy link
Contributor Author

@PacificGilly @farridav Ready :)

@PavelPancocha
Copy link
Contributor Author

Just Ruff failed for python 3.12 -> it could be formatted differently, but that doesn't work for python 3.8 - so I decided to keep it python 3.8 compatible. Maybe it could be set to always format it for lowest supported version?

@farridav
Copy link
Owner

farridav commented Apr 8, 2024

This looks like it's failing on the coveralls job, which is the only one that appears to be running ruff over the tests folder.. so it seems the formatting issue is in there..

Are you sure this build cannot be fixed ?

@PavelPancocha
Copy link
Contributor Author

@farridav I can remove the typing.

@PavelPancocha PavelPancocha force-pushed the fix/warning_logger_when_imagefield_is_empty branch 2 times, most recently from bb4ced1 to a61a48f Compare April 8, 2024 13:48
@PavelPancocha PavelPancocha force-pushed the fix/warning_logger_when_imagefield_is_empty branch from a61a48f to 11ca70e Compare April 8, 2024 13:49
@PavelPancocha
Copy link
Contributor Author

@farridav I fixed it, now fails with:

Not on TravisCI. You have to provide either repo_token in .coveralls.yml or set the COVERALLS_REPO_TOKEN env var

@farridav
Copy link
Owner

farridav commented Apr 8, 2024

Nice.. yes this particular error is with me, I need to make it so that the secret is available for other people.. but that doesn't block this PR as it will work fine in main.

Thanks for your contribution

@farridav farridav merged commit dc355ba into farridav:master Apr 8, 2024
4 of 5 checks passed
@jcgiuffrida
Copy link

For anyone else who is still dealing with this warning after upgrading to 3.0.0, your user_avatar can't be a field on a related model because it'll silently fail to fetch, then warn you that you have no avatar field. Use a @property on your User model to reference the avatar instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PacificGilly PacificGilly Awaiting requested review from PacificGilly

@jamesgilmorelyst jamesgilmorelyst Awaiting requested review from jamesgilmorelyst

@farridav farridav Awaiting requested review from farridav

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warning errors for unset avatar field on user model
5 participants
@PavelPancocha @farridav @jcgiuffrida @PacificGilly @jamesgilmorelyst