search

OCA / hr-expense

Human Resources Expenses OCA modules for Odoo
GNU Affero General Public License v3.0
50 stars 103 forks source link

Key error in group accces #237

Closed caber closed 3 months ago

caber commented 3 months ago

https://github.com/OCA/hr-expense/blob/ef75d4c00f9b039c420bc3bcc583fdf37b0d6f7b/hr_expense_invoice/models/hr_expense_sheet.py#L19 fails when not in group maybe change it in

for sheet in self.filtered(lambda rec: rec.id in res):

pedrobaeza commented 3 months ago

For which Odoo version? Have you checked https://github.com/OCA/hr-expense/pull/234?

pedrobaeza commented 3 months ago

No answer, so closing.

caber commented 3 months ago

Hi, Pedro, how are you doing? I understand your position, I proposed a one liner patch, which logic didin't account for a dirty workaround but to a logic and solid bugfix, if there's 0 amount entries, it doesn't make any sense to try to create new movements to conciliate those sheets, so they should be filtered out.

Besides that I've not having time or comprehension to understand fully #234 pr, that's way more complicated and with a much broader reach and objectives. Having said that, you're the maintainer and that's your call, but IMHO it doesn't make any service to the bigger objective to let the ecosystem grow and welcome newcomers and their contributions, and, in the long run it's not the way to foster community, and let odoo grow.

Cheers from uruguay, Luis

El mar, 28 may 2024 a la(s) 4:35 a.m., Pedro M. Baeza ( @.***) escribió:

No answer, so closing.

— Reply to this email directly, view it on GitHub https://github.com/OCA/hr-expense/issues/237#issuecomment-2134538467, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC6646I2KSMD6MK3LLQQWLZEQXV3AVCNFSM6AAAAABIEARISCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZUGUZTQNBWG4 . You are receiving this because you authored the thread.Message ID: @.***>

caber commented 3 months ago

The issue it referenced the commit, which was the last released one in main, at the time of publication. And no, I having being able the pr's logic, partly cause it's much broader reach.

234 it's a feature PR, my proposal would be merely a minimal bugfix,

different beasts altogether. Thx for your response, neverthe less. Cheers from Uruguay

El jue, 23 may 2024 a la(s) 3:19 a.m., Pedro M. Baeza ( @.***) escribió:

For which Odoo version? Have you checked #234 https://github.com/OCA/hr-expense/pull/234?

— Reply to this email directly, view it on GitHub https://github.com/OCA/hr-expense/issues/237#issuecomment-2126318337, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC6645R2ZAO7MMAFXXETUDZDWC63AVCNFSM6AAAAABIEARISCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRWGMYTQMZTG4 . You are receiving this because you authored the thread.Message ID: @.***>

pedrobaeza commented 3 months ago

It's better to directly say that is for version Odoo 14. The PR is for version 16, so there's no relation. You can propose a PR doing that one liner patch. I didn't want to discourage you, just to keep things clean if no answer.

caber commented 3 months ago

The problem was present in all the releases, as the code didn’t have changed

El El mar, 28 de may. de 2024 a la(s) 13:16, Pedro M. Baeza < @.***> escribió:

It's better to directly say that is for version Odoo 14. The PR is for version 16, so there's no relation. You can propose a PR doing that one liner patch. I didn't want to discourage you, just to keep things clean if no answer.

— Reply to this email directly, view it on GitHub https://github.com/OCA/hr-expense/issues/237#issuecomment-2135639490, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC6643ZIUSQ3PCHX5R7OUDZESUVDAVCNFSM6AAAAABIEARISCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZVGYZTSNBZGA . You are receiving this because you authored the thread.Message ID: @.***>

pedrobaeza commented 3 months ago

Well, res contains a recordset of account.move, so I don't see why for sheet in self.filtered(lambda rec: rec.id in res): should be correct, comparing hr.expense.sheet with account.move. Or am I missing something? Moreover, it would be interesting to have the steps to reproduce the problem.

caber commented 3 months ago

res is a dict, which keys are sheets ids, and move ids as values. If you have some invoiced sheet which invoice has amount 0, it does not create any move, so res doesn't have the key, so there's a key error at res[sheet.id]

El mar., 28 de may. de 2024 14:00, Pedro M. Baeza @.***> escribió:

Well, res contains a recordset of account.move, so I don't see why for sheet in self.filtered(lambda rec: rec.id in res): should be correct, comparing hr.expense.sheet with account.move. Or am I missing something? Moreover, it would be interesting to have the steps to reproduce the problem.

— Reply to this email directly, view it on GitHub https://github.com/OCA/hr-expense/issues/237#issuecomment-2135726265, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC6645LVMZ5FZK6G3SZTDLZESZ4LAVCNFSM6AAAAABIEARISCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZVG4ZDMMRWGU . You are receiving this because you authored the thread.Message ID: @.***>

pedrobaeza commented 3 months ago

OK, understood, although is a very rough case. Having an invoice of 0 amount seems weird. Anyway, please do the PR putting such patch.

caber commented 3 months ago

Ok, it's that it would be my first in github. Besides, I believe that the case could be an error, or maybe it could happen if a partner received free goods, related to the company. I'll be doing it later today. Greets from Uruguay, Luis

El El jue, 30 de may. de 2024 a la(s) 04:19, Pedro M. Baeza < @.***> escribió:

OK, understood, although is a very rough case. Having an invoice of 0 amount seems weird. Anyway, please do the PR putting such patch.

— Reply to this email directly, view it on GitHub https://github.com/OCA/hr-expense/issues/237#issuecomment-2138845758, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC6645E6BLYVBOJDFHUKXTZE3HJNAVCNFSM6AAAAABIEARISCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZYHA2DKNZVHA . You are receiving this because you authored the thread.Message ID: @.***>

pedrobaeza commented 3 months ago

A expense with a linked invoice with free goods? Seems very weird, but indeed the hole is there. Check https://odoo-community.org/resources/code for how to contribute.