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

fix(MR-16): Use cache for FileField and ImageField #17

Merged
merged 21 commits into from
Feb 27, 2021

Conversation

TrangPham
Copy link
Owner

@TrangPham TrangPham commented Feb 22, 2021

So this took a very long time to get to. Here's the previous history #16. This is related to #8

That worked nicely for ManyToManyField but not for the ImageField or FileField.

This MR tries caching (which works for FileField and ImageField, but not ManyToManyField).... thus a combined solution is used. Cache is used primarily, but for any m2m/generic relation fields data it looked up from request.POST (sent via hidden embedded form) and saved the related models.

Note: Had to bypass _save_related and save_m2m but calls the underlaying functions

Tests have been added for ManyToMany, File, and Image fields and caching

@TrangPham TrangPham force-pushed the MR16-test-image-field branch 2 times, most recently from 87f4e08 to f28e90d Compare February 22, 2021 08:22
admin_confirm/admin.py Outdated Show resolved Hide resolved
admin_confirm/admin.py Outdated Show resolved Hide resolved
admin_confirm/templates/admin/change_confirmation.html Outdated Show resolved Hide resolved
@TrangPham
Copy link
Owner Author

Previously in 0.2.3.dev6 relied mostly on the cache and handled m2m fields as a special case.
Now in 0.2.3.dev7 relying mostly on the post data and using cache to treat files and images as special cases.

Also added handy dandy tests readme to outline all the mutations that should be covered in test cases.

🤞 That this works better because .dev6 introduced more issues than it fixed.

@TrangPham
Copy link
Owner Author

0.2.3.dev9 is on testpypi. See: #8 (comment) For more details of differences between dev7 and dev9

@TrangPham TrangPham added this to In Review in KanBan Feb 25, 2021
README.md Outdated Show resolved Hide resolved
admin_confirm/admin.py Outdated Show resolved Hide resolved

if initial_value != new_value:
changed_data[name] = [initial_value, new_value]
if add:
Copy link
Owner Author

Choose a reason for hiding this comment

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

change_data for add and change_data for change could be refactors into helper methods to make this easier to read. Perhaps create a utils file for these Or add them to the existing one.

This would also also for better unit testing

admin_confirm/admin.py Show resolved Hide resolved
admin_confirm/admin.py Show resolved Hide resolved

# Should have cleared cache
for key in CACHE_KEYS.values():
self.assertIsNone(cache.get(key))
Copy link
Owner Author

Choose a reason for hiding this comment

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

Maybe 🤔 create a _assert helper method for this. It's required in most tests, but... it's bad practice to have any assert which is not explicit in the test func body.

admin_confirm/tests/unit/test_confirm_save_actions.py Outdated Show resolved Hide resolved
admin_confirm/tests/unit/test_fieldsets.py Show resolved Hide resolved
admin_confirm/tests/unit/test_file_cache.py Outdated Show resolved Hide resolved
tests/market/admin/item_admin.py Outdated Show resolved Hide resolved
@TrangPham TrangPham merged commit 06d3e1a into main Feb 27, 2021
KanBan automation moved this from In Review to Done Feb 27, 2021
@TrangPham TrangPham deleted the MR16-test-image-field branch February 27, 2021 23:39
@TrangPham TrangPham restored the MR16-test-image-field branch May 20, 2023 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

1 participant