-
-
Notifications
You must be signed in to change notification settings - Fork 507
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
[9.0][MIG] payment_redsys: Migration to v9 #406
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
@sergio-teruel, vas a poder añadir tests aquí? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review.
payment_redsys/models/redsys.py
Outdated
if website_id: | ||
base_url = '%s://%s' % ( | ||
request.httprequest.environ['wsgi.url_scheme'], | ||
self.env['website'].browse(website_id).domain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No estoy seguro, pero creo que esto podría llegar a fallar en algunas configuraciones de proxy inverso.
No es suficiente con usar web.base.url
?
O quizás añadir soporte para otro parámetro (ej. redsys.host
) y usar ese si está disponible y web.base.url
en caso contrario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No es suficiente porque no se sabe el protocolo, ya que ese parámetro sólo incluye el resto de la URL. No es una URI completa. Sergio ya hizo bastantes pruebas y no fallaba.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bueno, si viéramos que surgen problemas, ya se corregirán. 👍
payment_redsys/models/redsys.py
Outdated
_redsys_valid_tx_status = list(range(0, 100)) | ||
_redsys_pending_tx_status = list(range(101, 203)) | ||
_redsys_cancel_tx_status = [912, 9912] | ||
_redsys_error_tx_status = list(range(9064, 9095)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Como estás usando estas variables para comparar con el operador in
, mejor usa set
en vez de list
. En cualquier caso, esta implementación penaliza el rendimiento conforme a la anterior.
Creo que lo mejor sería un método parecido a:
def _redsys_status(self, code):
if 0 <= code <= 100:
return "done"
if 101 <= code <= 203:
return "pending"
... etc
Hola, este PR está funcionando correctamente hace meses. El hecho que no hacer merge creo que es negativo porque no aparece en el apps store ni como módulo y hace que menos gente lo utilice y pueda mejorarlo o incluso plantearse la migración a v10. Voto por merge ya. |
+1
y posiblemente lo migraríamos a v10.
…-----------------------------------------------------------
Josean Soroa
www.landoo.es
-----------------------------------------------------------
2017-04-02 10:31 GMT+02:00 Rafael Blasco <notifications@github.com>:
Hola, este PR está funcionando correctamente hace meses. El hecho que no
hacer merge creo que es negativo porque no aparece en el apps store ni como
módulo y hace que menos gente lo utilice y pueda mejorarlo o incluso
plantearse la migración a v10. Voto por merge ya.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#406 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AM296aKlUtI2netl0MvzMDDXinjyRRBTks5rr1zagaJpZM4LCfqz>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comments are still not addressed, but since they are only performance-related, I won't block this for that.
Pedro, se puede hacer merge de esto? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
👍
Veo que Redsys publica en su web los addons de software como Prestashop, Magento, WooCommerce, etc. |
@js-landoo, el módulo no lo voy a mergear hasta tener tests, pero puedes utilizarlo como quieras. Para lo otro, escribe en el proyecto correspondiente. |
OK, Pedro, tomo nota de la necesidad de tests. Lo miro. Por otro lado, disculpa mi desconocimiento pero no se a que te refieres con lo del proyecto correspondiente. En github no veo que haya proyectos en este repositorio y en la OCA tampoco. |
OCA/connector-magento, OCA/connector-prestashop |
Te refieres a la lista de correo Disculpa por ser tan pesado. Gracias. |
Perdona, que me he liado yo al creer que era al revés (en la página de Magento, referenciar a Redsys). Entonces sí, por favor, contacta con ellos a ver. |
OK, lo pongo en marcha : -) |
27f5aee
to
4be1ffc
Compare
Veo que esto tiene ya 3 aprobaciones, ¿falta algo para mezclar? |
No quiero mergear esto sin tener los correspondientes tests. @sergio-teruel estuvo trabajando en los tests para v10, que luego se puede hacer backport. Puedes echarle una mano con ellos si quieres. |
Hola Buenos días Un saludo Daniel Witoszek Arias |
@witoszek debes descargarte el código de este PR si quieres utilizarlo, y para eso necesitas conocimientos de GIT/GitHub que aquí no podemos darte. |
payment_redsys/models/redsys.py
Outdated
callback_url = self.env['ir.config_parameter'].get_param( | ||
'payment_redsys.callback_url') | ||
if callback_url: | ||
return '%s%s' % (callback_url, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En caso de llamarlo sin path como aquí https://github.com/OCA/l10n-spain/pull/406/files#diff-e6553d240f9a320224682ba5da058208R129 devolverá el valor de payment_redsys.callback_url+'None'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cámbialo por return '%s%s' % (callback_url, path or '')
abfeb41
to
805ab9d
Compare
95ea441
to
0a19d9f
Compare
@sergio-teruel Travis está en rojo |
0a19d9f
to
9f706ac
Compare
@pedrobaeza no consigo silenciar el log en esta funcion 9f706ac#diff-ad3fabe3e9d7083da3cc550d9e941c7aR209 |
No sé si será porque igual el mute_logger no funciona en peticiones http, donde se abre teóricamente otro hilo diferente. Si es así, tal vez puedas usar mock para comprobar que se llama a la excepción. |
Bueno, ahora mismo el PR está en verde, así que realmente no te está fastidiando, ¿verdad? ¿O es que en el último commit no están todos los tests? De todas formas, puede que te estés complicando la vida: cuando se lanzan excepciones, lo suyo sería comprobar esas excepciones con |
@pedrobaeza No. eso no funciona, el raise no es capturado |
Claro, no sirve cuando son peticiones http |
Hola, Saludos, Error :
Odoo Server Error
Traceback (most recent call last):
File "/usr/local/lib/python2.7/site-packages/openerp/http.py", line 647, in _handle_exception
return super(JsonRequest, self)._handle_exception(exception)
File "/usr/local/lib/python2.7/site-packages/openerp/http.py", line 684, in dispatch
result = self._call_function(**self.params)
File "/usr/local/lib/python2.7/site-packages/openerp/http.py", line 320, in _call_function
return checked_call(self.db, *args, **kwargs)
File "/usr/local/lib/python2.7/site-packages/openerp/service/model.py", line 118, in wrapper
return f(dbname, *args, **kwargs)
File "/usr/local/lib/python2.7/site-packages/openerp/http.py", line 313, in checked_call
result = self.endpoint(*a, **kw)
File "/usr/local/lib/python2.7/site-packages/openerp/http.py", line 963, in __call__
return self.method(*args, **kw)
File "/usr/local/lib/python2.7/site-packages/openerp/http.py", line 513, in response_wrap
response = f(*args, **kw)
File "/usr/local/lib/python2.7/site-packages/openerp/addons/web/controllers/main.py", line 901, in call_button
action = self._call_kw(model, method, args, {})
File "/usr/local/lib/python2.7/site-packages/openerp/addons/web/controllers/main.py", line 889, in _call_kw
return getattr(request.registry.get(model), method)(request.cr, request.uid, *args, **kwargs)
File "/usr/local/lib/python2.7/site-packages/openerp/api.py", line 250, in wrapper
return old_api(self, *args, **kwargs)
File "/usr/local/lib/python2.7/site-packages/openerp/addons/base/module/module.py", line 459, in button_immediate_install
return self._button_immediate_function(cr, uid, ids, self.button_install, context=context)
File "/usr/local/lib/python2.7/site-packages/openerp/api.py", line 250, in wrapper
return old_api(self, *args, **kwargs)
File "/usr/local/lib/python2.7/site-packages/openerp/addons/base/module/module.py", line 533, in _button_immediate_function
registry = openerp.modules.registry.RegistryManager.new(cr.dbname, update_module=True)
File "/usr/local/lib/python2.7/site-packages/openerp/modules/registry.py", line 386, in new
openerp.modules.load_modules(registry._db, force_demo, status, update_module)
File "/usr/local/lib/python2.7/site-packages/openerp/modules/loading.py", line 338, in load_modules
loaded_modules, update_module)
File "/usr/local/lib/python2.7/site-packages/openerp/modules/loading.py", line 237, in load_marked_modules
loaded, processed = load_module_graph(cr, graph, progressdict, report=report, skip_modules=loaded_modules, perform_checks=perform_checks)
File "/usr/local/lib/python2.7/site-packages/openerp/modules/loading.py", line 156, in load_module_graph
_load_data(cr, module_name, idref, mode, kind='data')
File "/usr/local/lib/python2.7/site-packages/openerp/modules/loading.py", line 98, in _load_data
tools.convert_file(cr, module_name, filename, idref, mode, noupdate, kind, report)
File "/usr/local/lib/python2.7/site-packages/openerp/tools/convert.py", line 851, in convert_file
convert_xml_import(cr, module, fp, idref, mode, noupdate, report)
File "/usr/local/lib/python2.7/site-packages/openerp/tools/convert.py", line 938, in convert_xml_import
obj.parse(doc.getroot(), mode=mode)
File "/usr/local/lib/python2.7/site-packages/openerp/tools/convert.py", line 801, in parse
self.parse(rec, mode)
File "/usr/local/lib/python2.7/site-packages/openerp/tools/convert.py", line 804, in parse
self._tags[rec.tag](self.cr, rec, de, mode=mode)
File "/usr/local/lib/python2.7/site-packages/openerp/tools/convert.py", line 708, in _tag_record
id = self.pool['ir.model.data']._update(cr, self.uid, rec_model, self.module, res, rec_id or False, not self.isnoupdate(data_node), noupdate=self.isnoupdate(data_node), mode=self.mode, context=rec_context )
File "/usr/local/lib/python2.7/site-packages/openerp/api.py", line 250, in wrapper
return old_api(self, *args, **kwargs)
File "/usr/local/lib/python2.7/site-packages/openerp/addons/base/ir/ir_model.py", line 1143, in _update
res_id = model_obj.create(cr, uid, values, context=context)
File "/usr/local/lib/python2.7/site-packages/openerp/api.py", line 250, in wrapper
return old_api(self, *args, **kwargs)
File "/usr/local/lib/python2.7/site-packages/openerp/addons/base/ir/ir_ui_view.py", line 353, in create
context=context)
File "/usr/local/lib/python2.7/site-packages/openerp/api.py", line 250, in wrapper
return old_api(self, *args, **kwargs)
File "/usr/local/lib/python2.7/site-packages/openerp/api.py", line 354, in old_api
result = method(recs, *args, **kwargs)
File "/usr/local/lib/python2.7/site-packages/openerp/models.py", line 4157, in create
record = self.browse(self._create(old_vals))
File "/usr/local/lib/python2.7/site-packages/openerp/api.py", line 248, in wrapper
return new_api(self, *args, **kwargs)
File "/usr/local/lib/python2.7/site-packages/openerp/api.py", line 490, in new_api
result = method(self._model, cr, uid, *args, **old_kwargs)
File "/usr/local/lib/python2.7/site-packages/openerp/models.py", line 4354, in _create
recs._validate_fields(vals)
File "/usr/local/lib/python2.7/site-packages/openerp/api.py", line 248, in wrapper
return new_api(self, *args, **kwargs)
File "/usr/local/lib/python2.7/site-packages/openerp/models.py", line 1271, in _validate_fields
raise ValidationError('\n'.join(errors))
ParseError: "Invalid view definition
Error details: Error context: |
@jesusmendezQubiq debes tener alguna modificación custom que lo estropea. Prueba sobre una BD en blanco. |
@pedrobaeza Módulo funcionando correctamente en BD en blanco. |
9f706ac
to
0ad0693
Compare
018c060
to
f4e5c7a
Compare
If there's a website installed in your integration environment, it will most likely have a `website_id` key that would produce a failure.
f4e5c7a
to
522a31b
Compare
Travis went ❌ with a8b488e |
Ya lo he visto y estoy con ello. Eso es lo que pasa por hacer rebase 😛 |
a8b488e
to
3491b66
Compare
Bueno, tras un largo camino, este módulo pasa a estar completo, con tests, y con una base grande de pruebas en varios clientes. Fusiono. |
Hola, soy nuevo en Odoo. ¿Cuánto queda para que el módulo Redsys aparezca en Odoo apps para la versión 10 (y 11)? |
Tovía no sale para v11 🤔 pero si para v10 https://www.odoo.com/apps/modules/11.0/payment_redsys/ |
cc @Tecnativa