itpp-labs / pos-addons

Odoo POS Addons
https://itpp.dev
MIT License
360 stars 586 forks source link

[pos_order_cancel] save pos.cancelled_reason.id in pos.order.line.canceled #404

Open gaelTorrecillas opened 7 years ago

gaelTorrecillas commented 7 years ago

hello,

it should be a great idea to save pos.cancelled_reason.id in pos.order.line.canceled

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/49627196-pos_order_cancel-save-pos-cancelled_reason-id-in-pos-order-line-canceled?utm_campaign=plugin&utm_content=tracker%2F2289114&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F2289114&utm_medium=issues&utm_source=github).
yelizariev commented 6 years ago

@GabbasovDinar the idea is to add fields.many2many('pos.cancelled_reason') to pos.order.line.canceled model

flotho commented 6 years ago

Hi @yelizariev , actually, you meant that we could have multiple cancelation reason on pos.order.line ? What we're trying to do is to plug BI and reports on cancelation reasons. having each line with a many2one could be more effective maybe?

yelizariev commented 6 years ago

actually, you meant that we could have multiple cancelation reason on pos.order.line ?

I meant multiple cancelation reason on pos.order.line.canceled

What we're trying to do is to plug BI and reports on cancelation reasons. having each line with a many2one could be more effective maybe?

Which many2one field do you mean?

We don't want to restrict module with one reason per line only.

I guess that your concern is that cancled line can be accounted twice in report if you have multiple reasons on that, isn't it? Probably, just add summary on all reasons at the report, e.g.

Line A: 500 - Reason 1
Line B: 500 - Reason 1 and Reason 2
Line C: 1000 - Reason 2

Then in report:


Reason 1: 1000 usd
Reason 2: 1500 usd

Total per Reason: 2500
Total: 2000
flotho commented 6 years ago

Hi @yelizariev,

Thanks for your answers,

I meant multiple cancelation reason on pos.order.line.canceled

You're right, I wrote too fast

I guess that your concern is that canceled line can be accounted twice in report if you have multiple reasons on that, isn't it? Probably, just add summary on all reasons at the report, e.g.

You are totally right regarding the need. I also understand your wish to allow multiple reason on a line. It's of course necessary to prevent any fraud etc...

Maybe we could have an intermediate way which could rely on an intermediate object linked to each line that store the amount and the reason. Thus, the line.cancelled has no multiple instance, the reason text could keep trace of all the reasons and from this line we could see all the reasons with the amount for each of them.

It could turn the model like this :

class PosOrderLineCancelationValue(models.Model):
            _name='pos.order.line.cancelation.value'
            reason_id = fields.Many2one(....)
            line_id = fields.Many2one("pos.order.line.canceled")
            untaxed_amount = ....
            tax_amount = ....
class PosOrderLineCanceled(models.Model):
            _name = "pos.order.line.canceled"
           cancelvalue_id = fields.One2many('pos.order.line.cancelation.value')   

Do you think it could represent a huge evolution?

flotho commented 6 years ago

My bad,

with @gaelTorrecillas we made some additionnal tests on your module . As far as we understand, the reason is asked once only when you start modifying the values. Then all the cancelled lines are sent to the server when the transaction is validated. So technically, your line.cancel has only one reason (in text from now): https://github.com/it-projects-llc/pos-addons/blob/10.0/pos_order_cancel/static/src/js/models.js#L44 Have we correctly understand this? If yes, wouldn't it be easier to store a many2one on the line.canceled if a cancellation reason is selected by end user? This way, the end user could continue to input free comment on reason so that it won't be restricted as you want and in the same way, if the end user use a cancellation reason, this particular value will be stored in a many2one.

flotho commented 6 years ago

Hi @GabbasovDinar , @yelizariev ,

what is your position on my latest comment https://github.com/it-projects-llc/pos-addons/issues/404#issuecomment-332189522 ?

Thanks for your effective support

gaelTorrecillas commented 6 years ago

Hi @GabbasovDinar , @yelizariev ,

during the test for the pr (https://github.com/it-projects-llc/pos-addons/pull/410), with @flotho we saw that in the point of sale, it's possible to select many reasons, we understand the choice of many2many field. It is no longer necessary to keep the amount for each reason.

yelizariev commented 6 years ago

So, we can continue on working implementation as I suggested?

Are there any other concerns we need to pay attention?

gaelTorrecillas commented 6 years ago

hello @yelizariev ,

yes, your seggestion is good, and for the second question, I need your opinion for this PR https://github.com/it-projects-llc/pos-addons/pull/410 and if it's possible to merge