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

[IMP] Getting rid of _quant_create method override from product_unique_serial module #14

Merged

Conversation

hbto
Copy link

@hbto hbto commented May 29, 2016

[IMP] Getting rid of _quant_create method override from product_unique_serial module
[IMP] Fixing tests on product_unique_serial module because of the delete override

@hbto
Copy link
Author

hbto commented May 29, 2016

@nhomar @moylop260

Method _quant_create just got deleted from this module and
modules test only got a few changes to fit with the new changes.

Could you please review and/or merge this PR

Regards

line.product_id.name, line.qty,
line.lot_id.name, note)))
'You should only receive by the piece with the same '
'serial number'))
Copy link
Member

Choose a reason for hiding this comment

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

As you see hbto you are not showing proper information of the product failling here.

Can you concatenate properly?

Copy link
Author

Choose a reason for hiding this comment

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

I am using the same lines that were used before,

https://github.com/Vauxoo/stock-logistics-workflow/pull/14/files#diff-740d05815bfc94b847d7e0d9d485423fL94

In order to be too much hassle the module's test itself and other test upwards

@nhomar
Copy link
Member

nhomar commented May 29, 2016

I like the new approach.

What We need is send the warning with all the information related (product and serial) involved in the warning.

@hbto
Copy link
Author

hbto commented May 29, 2016

As you see hbto you are not showing proper information of the product failling here.
Can you concatenate properly?

I am using the same approach used earlier by avoiding to break the least what is already working.

If some improve is needed I want to do that in a second round,

I am trying to reach an stable process, by changing the test & validation at all I will be causing
too much noise upwards in other modules and integrations in the customer side.

So, yes you are right, Exceptions and Test needs better wording, but that can be done
in a second round.

Regards

@hbto
Copy link
Author

hbto commented May 30, 2016

@nhomar Could you please review and/or merge


msg_increase = _(
u'Product %s has been configured to use unique lots. '
u'You are trying to increase %s items in the lot %s. %s')
Copy link
Member

Choose a reason for hiding this comment

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

Avoid global variables.

Can you please add them as attributes in the class and they will be available in all the class, use this kind of variables is a bad practice.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your advice.

Doing

Copy link
Author

@hbto hbto May 31, 2016

Choose a reason for hiding this comment

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

BTW

Do pylint or any other of our tools check for this bad practice,
or Have I overlooked the check?

@moylop260

@nhomar
Copy link
Member

nhomar commented May 30, 2016

For me is ok all (thanks) I just have a little comment in the global variables used for the long texts.

The correct approach is add them as new attributes to the class.

class name(models.Model):
    _your_var = _(----....long text)
    fields_name = fields.Text(help=self._your_var)
# in the method
    def method(self):
        ....
        self._your_var

@hbto
Copy link
Author

hbto commented May 31, 2016

@nhomar
Your latest advice has been taken into account.

Please recheck.

Regards

@nhomar
Copy link
Member

nhomar commented May 31, 2016

As a last step can you make a dummy PR to yoytec please.!

@hbto
Copy link
Author

hbto commented May 31, 2016

@nhomar
That is already running since first day at:
https://github.com/Vauxoo/yoytec/pull/1307

Regards

@hbto
Copy link
Author

hbto commented Jun 22, 2016

@nhomar

Please review runbot for yoytec on this PR.
http://runbot.vauxoo.com/runbot/static/build/12970-1307-eadbee/logs/job_20_test_all.txt

being this the regarding pull request
https://github.com/Vauxoo/yoytec/pull/1307

this has already gone green

return super(StockQuant, self)._quant_create(
qty, move, lot_id, owner_id, src_package_id,
dest_package_id, force_location_from, force_location_to)

@api.multi
@api.constrains('product_id', 'lot_id', 'qty')
def _check_inicity_lot_product(self):
Copy link
Member

@nhomar nhomar Jun 22, 2016

Choose a reason for hiding this comment

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

s/inicity/uniqueness/g

s/_check_inicity_lot_product/_check_uniqueness_lot/g

Copy link
Author

Choose a reason for hiding this comment

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

s/inicity/uniqueness/g

s/_check_inicity_lot_product/_check_uniqueness_lot/g

@nhomar
A long time ago that was taken care of

check #13

Regards

@nhomar
Copy link
Member

nhomar commented Jun 22, 2016

It is Ok for me, just a little typo to merge, can you @hugho-ad check this please to merge ASAP

@hugho-ad
Copy link
Collaborator

I'm not full in context for a reviewing, maybe you are askying for Moy's reviewing

Regards

@nhomar
Copy link
Member

nhomar commented Jun 22, 2016

Sorry I meant @hbto

@nhomar nhomar merged commit 033fce2 into Vauxoo:8.0 Jun 22, 2016
@nhomar nhomar deleted the 8.0-product_unique_serial-constraint-hbto branch June 22, 2016 14:24
@Vauxoo Vauxoo locked and limited conversation to collaborators Jun 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants