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: call _fetch_item from _process_image for easier override #4280

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Chadys
Copy link
Contributor

@Chadys Chadys commented Apr 22, 2024

In catalogue.Importer, we have the _fetch_item function as a practical override entry point.
But instead of using it, _process_image is reimplementing the same logic directly in the function.
I fixed it so that _fetch_item is used and I removed the now unnecessary lookup argument in _process_image.

With those changes, in handle L.41, the lookup_value is retrieved but is only used to construct some error logs. I think we should change the text of the logs so as not to need to retrieve this value just for them, but I wanted to discuss about it first.

(PS: I have been working using Oscar for more than 2 years now, building a marketplace from scratch. I finally have some times to give back to that awesome project by providing some fixes for issues I found along the way so I'll be pretty active in the following days!)

Copy link

codecov bot commented Apr 22, 2024

Codecov Report

Merging #4280 (7772c4c) into master (204b9a7) will increase coverage by 0.08%.
Report is 8 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4280      +/-   ##
==========================================
+ Coverage   88.28%   88.36%   +0.08%     
==========================================
  Files         293      293              
  Lines       16046    16093      +47     
==========================================
+ Hits        14166    14221      +55     
+ Misses       1880     1872       -8     
Files Coverage Δ
src/oscar/apps/catalogue/utils.py 0.00% <0.00%> (ø)

... and 9 files with indirect coverage changes

@viggo-devries
Copy link
Contributor

hi @Chadys thanks for your contributions!

We actually never use this Importer in production. see: https://github.com/django-oscar/django-oscar/blob/master/src/oscar/apps/catalogue/utils.py#L25 so I don't think will merge this.

Recently I have been working on a new project for easily mapping catalogue data from random input (in any format like json, xml, excel etc.) to product models, and from product models to any other format.

you should check out django-oscar-odin: https://github.com/django-oscar/django-oscar-odin

Could be really beneficial to your marketplace integrations as it makes creating products much faster (bulk updates and creates)

@Chadys
Copy link
Contributor Author

Chadys commented Apr 22, 2024

I never used the Importer in production either, still it was useful to create some fixtures that I used in development, and I actually have needed to override _fetch_item and ended up overriding _process_image for that. Hence this little fix. But I agree that it is not that important.

I'm not working for that marketplace anymore (that's the reason why I finally have some time to myself haha), but some utilities to map catalogue data would definitely have been useful!
For the marketplace what I did was to implement a whole bunch of utility classes to update the catalogue in bulk and easily handle data from different sources (API, FTP, ...) by only needing to declare the specific format for a seller in a pydantic model and have a generic way to handle them all. That took a lot of time to implement so your new project is definitely a welcome addition to the ecosystem, I'll check it!

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.

None yet

2 participants