Merge lp:~openerp-connector-core-editors/openerp-connector-magento/7.0-magentoerpconnect-404-image-1210543 into lp:~openerp-connector-core-editors/openerp-connector-magento/7.0

Proposed by Guewen Baconnier @ Camptocamp on 2013-09-02
Status: Merged
Merged at revision: 930
Proposed branch: lp:~openerp-connector-core-editors/openerp-connector-magento/7.0-magentoerpconnect-404-image-1210543
Merge into: lp:~openerp-connector-core-editors/openerp-connector-magento/7.0
Diff against target: 340 lines (+272/-15)
4 files modified
magentoerpconnect/product.py (+38/-15)
magentoerpconnect/tests/__init__.py (+2/-0)
magentoerpconnect/tests/test_data_product.py (+112/-0)
magentoerpconnect/tests/test_import_product_image.py (+120/-0)
To merge this branch: bzr merge lp:~openerp-connector-core-editors/openerp-connector-magento/7.0-magentoerpconnect-404-image-1210543
Reviewer Review Type Date Requested Status
Guewen Baconnier @ Camptocamp Abstain on 2013-09-03
Review via email: mp+183415@code.launchpad.net

Description of the change

Fixes lp:1210543

When an image is missing and the Magento HTTP Server returns a 404 error, we try to import the next image.

To post a comment you must log in.
908. By Guewen Baconnier @ Camptocamp on 2013-09-02

[FIX] remove print statements

I want to reduce the scope of the test: the test should cover the CatalogImageImporter, not the whole product import

review: Needs Fixing
909. By Guewen Baconnier @ Camptocamp on 2013-09-03

[IMP] The tested scope is now more precise for the import of the product's images, it does not import the full product, just the image

I put 'Abstain' as I'm the author and I just want to discard the 'Needs Fixing' flag I put along my name.

review: Abstain

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'magentoerpconnect/product.py'
2--- magentoerpconnect/product.py 2013-07-26 09:06:53 +0000
3+++ magentoerpconnect/product.py 2013-09-03 09:56:01 +0000
4@@ -24,7 +24,7 @@
5 import urllib2
6 import base64
7 import xmlrpclib
8-from operator import itemgetter
9+import sys
10 from openerp.osv import orm, fields
11 from openerp.tools.translate import _
12 from openerp.addons.connector.queue.job import job
13@@ -256,30 +256,53 @@
14 def _get_images(self, storeview_id=None):
15 return self.backend_adapter.get_images(self.magento_id, storeview_id)
16
17- def _get_main_image_data(self, images):
18+ def _sort_images(self, images):
19+ """ Returns a list of images sorted by their priority.
20+ An image with the 'base' type is the the primary one.
21+ The other images are sorted by their position.
22+
23+ The returned list is reversed, the items at the end
24+ of the list have the higher priority.
25+ """
26 if not images:
27 return {}
28- # search if we have a base image defined
29- image = next((img for img in images if 'base' in img['types']),
30- None)
31- if image is None:
32- # take the first image by priority
33- images = sorted(images, key=itemgetter('position'))
34- image = images[0]
35- return image
36+ # place the images where the type is 'base' first then
37+ # sort them by the reverse priority (last item of the list has
38+ # the the higher priority)
39+ def priority(image):
40+ primary = 'base' in image['types']
41+ try:
42+ position = int(image['position'])
43+ except ValueError:
44+ position = sys.maxint
45+ return (primary, -position)
46+ return sorted(images, key=priority)
47
48 def _get_binary_image(self, image_data):
49 url = image_data['url']
50- binary = urllib2.urlopen(url)
51- return binary.read()
52+ try:
53+ binary = urllib2.urlopen(url)
54+ except urllib2.HTTPError as err:
55+ if err.code == 404:
56+ # the image is just missing, we skip it
57+ return
58+ else:
59+ # we don't know why we couldn't download the image
60+ # so we propagate the error, the import will fail
61+ # and we have to check why it couldn't be accessed
62+ raise
63+ else:
64+ return binary.read()
65
66 def run(self, magento_id, binding_id):
67 self.magento_id = magento_id
68 images = self._get_images()
69- main_image_data = self._get_main_image_data(images)
70- if not main_image_data:
71+ images = self._sort_images(images)
72+ binary = None
73+ while not binary and images:
74+ binary = self._get_binary_image(images.pop())
75+ if not binary:
76 return
77- binary = self._get_binary_image(main_image_data)
78 with self.session.change_context({'connector_no_export': True}):
79 self.session.write(self.model._name,
80 binding_id,
81
82=== modified file 'magentoerpconnect/tests/__init__.py'
83--- magentoerpconnect/tests/__init__.py 2013-08-09 21:06:40 +0000
84+++ magentoerpconnect/tests/__init__.py 2013-09-03 09:56:01 +0000
85@@ -22,6 +22,7 @@
86 import test_synchronization
87 import test_address_book
88 import test_export_invoice
89+import test_import_product_image
90
91 fast_suite = [
92 ]
93@@ -30,4 +31,5 @@
94 test_synchronization,
95 test_address_book,
96 test_export_invoice,
97+ test_import_product_image,
98 ]
99
100=== added file 'magentoerpconnect/tests/test_data_product.py'
101--- magentoerpconnect/tests/test_data_product.py 1970-01-01 00:00:00 +0000
102+++ magentoerpconnect/tests/test_data_product.py 2013-09-03 09:56:01 +0000
103@@ -0,0 +1,112 @@
104+# -*- coding: utf-8 -*-
105+##############################################################################
106+#
107+# Author: Guewen Baconnier
108+# Copyright 2013 Camptocamp SA
109+#
110+# This program is free software: you can redistribute it and/or modify
111+# it under the terms of the GNU Affero General Public License as
112+# published by the Free Software Foundation, either version 3 of the
113+# License, or (at your option) any later version.
114+#
115+# This program is distributed in the hope that it will be useful,
116+# but WITHOUT ANY WARRANTY; without even the implied warranty of
117+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
118+# GNU Affero General Public License for more details.
119+#
120+# You should have received a copy of the GNU Affero General Public License
121+# along with this program. If not, see <http://www.gnu.org/licenses/>.
122+#
123+##############################################################################
124+
125+"""
126+Magento responses for calls done by the connector.
127+
128+This set of responses has been recorded for the synchronizations
129+with a Magento 1.7 version with demo data.
130+
131+It has been recorded using ``magentoerpconnect.unit.backend_adapter.record``
132+and ``magentoerpconnect.unit.backend_adapter.output_recorder``
133+
134+This set of data contains examples of imported products.
135+"""
136+import urllib2
137+
138+
139+# a simple product with images
140+simple_product_and_images = {
141+ ('catalog_product.info', (122, None, None, 'id')): {'categories': ['1'],
142+ 'color': '60',
143+ 'cost': '2.0000',
144+ 'country_of_manufacture': None,
145+ 'created_at': '2007-08-24 19:53:06',
146+ 'custom_design': '',
147+ 'custom_design_from': None,
148+ 'custom_design_to': None,
149+ 'custom_layout_update': '',
150+ 'description': "We bought these with the intention of making shirts for our family reunion, only to come back the next day to find each and every one of them had been tagged by The Bear. Oh well -- can't argue with art. Now you can make your grandparents proud by wearing an original piece of graf work to YOUR family reunion!",
151+ 'enable_googlecheckout': '0',
152+ 'gender': '35',
153+ 'gift_message_available': '',
154+ 'group_price': [],
155+ 'has_options': '0',
156+ 'image_label': None,
157+ 'is_recurring': '0',
158+ 'manufacturer': None,
159+ 'meta_description': 'Ink Eater: Krylon Bombear Destroyed Tee',
160+ 'meta_keyword': 'Ink Eater: Krylon Bombear Destroyed Tee',
161+ 'meta_title': 'Ink Eater: Krylon Bombear Destroyed Tee',
162+ 'minimal_price': '22.0000',
163+ 'model': 'Ink Eater:',
164+ 'msrp': None,
165+ 'msrp_display_actual_price_type': '4',
166+ 'msrp_enabled': '2',
167+ 'name': 'Ink Eater: Krylon Bombear Destroyed Tee',
168+ 'news_from_date': None,
169+ 'news_to_date': None,
170+ 'old_id': None,
171+ 'options_container': 'container2',
172+ 'page_layout': None,
173+ 'price': '22.0000',
174+ 'product_id': '122',
175+ 'recurring_profile': None,
176+ 'required_options': '0',
177+ 'set': '41',
178+ 'shirt_size': '98',
179+ 'short_description': "We bought these with the intention of making shirts for our family reunion, only to come back the next day to find each and every one of them had been tagged by The Bear. Oh well -- can't argue with art. Now you can make your grandparents proud by wearing an original piece of graf work to YOUR family reunion!",
180+ 'sku': 'ink_lrg',
181+ 'small_image_label': None,
182+ 'special_from_date': None,
183+ 'special_price': None,
184+ 'special_to_date': None,
185+ 'status': '1',
186+ 'tax_class_id': '2',
187+ 'thumbnail_label': None,
188+ 'tier_price': [],
189+ 'type': 'simple',
190+ 'type_id': 'simple',
191+ 'updated_at': '2013-09-02 08:01:34',
192+ 'url_key': 'ink-eater-krylon-bombear-destroyed-tee-lrg',
193+ 'url_path': 'ink-eater-krylon-bombear-destroyed-tee-lrg.html',
194+ 'visibility': '1',
195+ 'websites': ['1'],
196+ 'weight': '0.5000'},
197+ ('product_media.list', (122, None, 'id')): [{'exclude': '1',
198+ 'file': '/i/n/ink-eater-krylon-bombear-destroyed-tee-2.jpg',
199+ 'label': '',
200+ 'position': '0',
201+ 'types': ['thumbnail'],
202+ 'url': 'http://localhost:9100/media/catalog/product/i/n/ink-eater-krylon-bombear-destroyed-tee-2.jpg'},
203+ {'exclude': '0',
204+ 'file': '/i/n/ink-eater-krylon-bombear-destroyed-tee-1.jpg',
205+ 'label': '',
206+ 'position': '3',
207+ 'types': ['small_image'],
208+ 'url': 'http://localhost:9100/media/catalog/product/i/n/ink-eater-krylon-bombear-destroyed-tee-1.jpg'},
209+ {'exclude': '0',
210+ 'file': '/m/a/magentoerpconnect_1.png',
211+ 'label': '',
212+ 'position': '4',
213+ 'types': [],
214+ 'url': 'http://localhost:9100/media/catalog/product/m/a/magentoerpconnect_1.png'}],
215+}
216
217=== added file 'magentoerpconnect/tests/test_import_product_image.py'
218--- magentoerpconnect/tests/test_import_product_image.py 1970-01-01 00:00:00 +0000
219+++ magentoerpconnect/tests/test_import_product_image.py 2013-09-03 09:56:01 +0000
220@@ -0,0 +1,120 @@
221+# -*- coding: utf-8 -*-
222+##############################################################################
223+#
224+# Author: Guewen Baconnier
225+# Copyright 2013 Camptocamp SA
226+#
227+# This program is free software: you can redistribute it and/or modify
228+# it under the terms of the GNU Affero General Public License as
229+# published by the Free Software Foundation, either version 3 of the
230+# License, or (at your option) any later version.
231+#
232+# This program is distributed in the hope that it will be useful,
233+# but WITHOUT ANY WARRANTY; without even the implied warranty of
234+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
235+# GNU Affero General Public License for more details.
236+#
237+# You should have received a copy of the GNU Affero General Public License
238+# along with this program. If not, see <http://www.gnu.org/licenses/>.
239+#
240+##############################################################################
241+
242+import urllib2
243+import mock
244+from base64 import b64encode
245+
246+from openerp.addons.magentoerpconnect.unit.import_synchronizer import (
247+ import_batch, import_record)
248+from openerp.addons.connector.session import ConnectorSession
249+import openerp.tests.common as common
250+from .common import mock_api, MockResponseImage
251+from .test_data import magento_base_responses
252+from .test_data_product import simple_product_and_images
253+from openerp.addons.magentoerpconnect.product import (
254+ CatalogImageImporter, ProductProductAdapter)
255+
256+# simple square of 4 px filled with green in png, used for the product
257+# images
258+PNG_IMG_4PX_GREEN = "\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x04\x00\x00\x00\x04\x08\x02\x00\x00\x00&\x93\t)\x00\x00\x00\x01sRGB\x00\xae\xce\x1c\xe9\x00\x00\x00\tpHYs\x00\x00\x0b\x13\x00\x00\x0b\x13\x01\x00\x9a\x9c\x18\x00\x00\x00\x07tIME\x07\xdd\t\x02\t\x1d0\x13\th\x94\x00\x00\x00\x19tEXtComment\x00Created with GIMPW\x81\x0e\x17\x00\x00\x00\x12IDAT\x08\xd7cd\xf8\xcf\x00\x07L\x0c\x0c\xc4p\x002\xd2\x01\x07\xce\xee\xd0\xcf\x00\x00\x00\x00IEND\xaeB`\x82"
259+B64_PNG_IMG_4PX_GREEN = b64encode(PNG_IMG_4PX_GREEN)
260+
261+
262+class test_import_product_image(common.TransactionCase):
263+ """ Test the imports of the image of the products. """
264+
265+ def setUp(self):
266+ super(test_import_product_image, self).setUp()
267+ backend_model = self.registry('magento.backend')
268+ warehouse_id = self.ref('stock.warehouse0')
269+ self.backend_id = backend_model.create(
270+ self.cr,
271+ self.uid,
272+ {'name': 'Test Magento',
273+ 'version': '1.7',
274+ 'location': 'http://anyurl',
275+ 'username': 'guewen',
276+ 'warehouse_id': warehouse_id,
277+ 'password': '42'})
278+
279+ self.session = ConnectorSession(self.cr, self.uid)
280+ with mock_api(magento_base_responses):
281+ import_batch(self.session, 'magento.website', self.backend_id)
282+ import_batch(self.session, 'magento.store', self.backend_id)
283+ import_batch(self.session, 'magento.storeview', self.backend_id)
284+ import_record(self.session, 'magento.product.category',
285+ self.backend_id, 1)
286+ self.session = ConnectorSession(self.cr, self.uid)
287+ self.product_model = self.registry('magento.product.product')
288+
289+ def test_image_priority(self):
290+ """ Check if the images are sorted in the correct priority """
291+ env = mock.Mock()
292+ importer = CatalogImageImporter(env)
293+ file1 = {'file': 'file1', 'types': ['base'], 'position': '10'}
294+ file2 = {'file': 'file2', 'types': ['thumbnail'], 'position': '3'}
295+ file3 = {'file': 'file3', 'types': ['thumbnail'], 'position': '4'}
296+ file4 = {'file': 'file4', 'types': [], 'position': '10'}
297+ images = [file2, file1, file4, file3]
298+ self.assertEquals(importer._sort_images(images),
299+ [file4, file3, file2, file1])
300+
301+ def test_import_images_404(self):
302+ """ Import a product when an image respond a 404 error, should skip and take the first valid """
303+ env = mock.MagicMock()
304+ env.get_connector_unit.return_value = ProductProductAdapter(env)
305+ importer = CatalogImageImporter(env)
306+ with mock.patch('urllib2.urlopen') as urlopen:
307+ def image_url_response(url):
308+ if url == 'http://localhost:9100/media/catalog/product/i/n/ink-eater-krylon-bombear-destroyed-tee-2.jpg':
309+ raise urllib2.HTTPError(url, 404, '404', None, None)
310+ elif url == 'http://localhost:9100/media/catalog/product/i/n/ink-eater-krylon-bombear-destroyed-tee-1.jpg':
311+ raise urllib2.HTTPError(url, 404, '404', None, None)
312+ else:
313+ return MockResponseImage(PNG_IMG_4PX_GREEN)
314+
315+ urlopen.side_effect = image_url_response
316+ with mock_api(simple_product_and_images):
317+ importer.run(122, 999)
318+
319+ env.session.write.assert_called_with(mock.ANY,
320+ 999,
321+ {'image': B64_PNG_IMG_4PX_GREEN})
322+
323+ def test_import_images_403(self):
324+ """ Import a product when an image respond a 403 error, should fail """
325+ env = mock.MagicMock()
326+ env.get_connector_unit.return_value = ProductProductAdapter(env)
327+ importer = CatalogImageImporter(env)
328+ with mock.patch('urllib2.urlopen') as urlopen:
329+ def image_url_response(url):
330+ if url == 'http://localhost:9100/media/catalog/product/i/n/ink-eater-krylon-bombear-destroyed-tee-2.jpg':
331+ raise urllib2.HTTPError(url, 404, '404', None, None)
332+ elif url == 'http://localhost:9100/media/catalog/product/i/n/ink-eater-krylon-bombear-destroyed-tee-1.jpg':
333+ raise urllib2.HTTPError(url, 403, '403', None, None)
334+ else:
335+ return MockResponseImage(PNG_IMG_4PX_GREEN)
336+
337+ urlopen.side_effect = image_url_response
338+ with mock_api(simple_product_and_images):
339+ with self.assertRaises(urllib2.HTTPError):
340+ importer.run(122, 999)