[18.0][IMP] rma: Allow users without RMA permissions to confirm stock moves not related to RMA - #614
[18.0][IMP] rma: Allow users without RMA permissions to confirm stock moves not related to RMA#614victoralmau wants to merge 1 commit into
Conversation
|
Hi @pedrobaeza, @chienandalu, |
| ] | ||
| extra_fields = [] | ||
| _self = self.sudo() | ||
| if _self.rma_id: |
There was a problem hiding this comment.
Is self multi-record? And if one to merge has RMA and the other not?
There was a problem hiding this comment.
Changed to ensure proper compatibility with multi-record.
In my opinion, it's not possible for some items to be linked to an RMA while others aren't; the picking (confirmed) is created from the RMA (inbound or outbound), and the user has RMA permissions.
… not related to RMA
73f391a to
82f7aac
Compare
| ] | ||
| extra_fields = [] | ||
| _self = self.sudo() | ||
| if any(move.rma_id for move in _self): |
There was a problem hiding this comment.
But why you need to do this? Simply add them always.
There was a problem hiding this comment.
Use case example:
- A user with inventory permissions but WITHOUT RMA permissions creates a picking (for outbound, for example) and attempts to confirm it.
The following error occurs:
File "/opt/odoo/auto/addons/stock/models/stock_picking.py", line 1177, in action_confirm
self.move_ids.filtered(lambda move: move.state == 'draft')._action_confirm()
File "/opt/odoo/auto/addons/stock/models/stock_move.py", line 1577, in _action_confirm
moves = self._merge_moves(merge_into=merge_into)
File "/opt/odoo/auto/addons/stock/models/stock_move.py", line 1219, in _merge_moves
for __, g in groupby(candidate_moves, key=self._merge_move_itemgetter(distinct_fields)):
File "/opt/odoo/custom/src/odoo/odoo/tools/misc.py", line 1216, in groupby
groups[key(elem)].append(elem)
File "/opt/odoo/auto/addons/stock/models/stock_move.py", line 1183, in <lambda>
return lambda move: base_getter(move) + tuple(_get_formatted_float_fields(move, f_name, float_precision) for f_name in float_fields)
File "/opt/odoo/custom/src/odoo/odoo/models.py", line 7083, in __getitem__
return self._fields[key].__get__(self)
File "/opt/odoo/custom/src/odoo/odoo/fields.py", line 4673, in __get__
return super().__get__(records, owner)
File "/opt/odoo/custom/src/odoo/odoo/fields.py", line 3111, in __get__
return super().__get__(records, owner)
File "/opt/odoo/custom/src/odoo/odoo/fields.py", line 1272, in __get__
recs._fetch_field(self)
File "/opt/odoo/custom/src/odoo/odoo/models.py", line 4120, in _fetch_field
self.fetch(fnames)
File "/opt/odoo/custom/src/odoo/odoo/models.py", line 4158, in fetch
fetched = self._fetch_query(query, fields_to_fetch)
File "/opt/odoo/custom/src/odoo/odoo/models.py", line 4269, in _fetch_query
field.read(fetched)
File "/opt/odoo/custom/src/odoo/odoo/fields.py", line 4688, in read
lines = comodel.search_fetch(domain, field_names)
File "/opt/odoo/custom/src/odoo/odoo/models.py", line 1778, in search_fetch
query = self._search(domain, offset=offset, limit=limit, order=order or self._order)
File "/opt/odoo/custom/src/odoo/odoo/models.py", line 5812, in _search
self.browse().check_access('read')
File "/opt/odoo/custom/src/odoo/odoo/models.py", line 4444, in check_access
raise result[1]()
odoo.exceptions.AccessError: You are not allowed to access 'RMA' (rma) records.
In my opinion, the best way to fix this issue is to ensure that if the record (stock.move) does not have any RMA data associated with it, those fields are not added to the _prepare_merge_moves_distinct_fields() method. This will prevent users without RMA permissions from performing processes that are NOT RMA-related.
The test that has been added specifically tests this use case.
There was a problem hiding this comment.
But I thought that the previous sudo already handles that. And now
| "rma_receiver_ids", | ||
| ] | ||
| extra_fields = [] | ||
| _self = self.sudo() |
There was a problem hiding this comment.
You can do: self = self.sudo(). It's a local variable.
| ] | ||
| extra_fields = [] | ||
| _self = self.sudo() | ||
| if any(move.rma_id for move in _self): |
There was a problem hiding this comment.
Thinking twice, if self.rma_id: serves as well, as it's just non falsy if one or several records exist.
There was a problem hiding this comment.
But coming again to the main concept, I think this is done for seeing if a move can be merged with other existing ones, so isn't possible that one has rma_id and the other not? You may do a return without RMA.
There was a problem hiding this comment.
Even so, only if one of them (stock.move) has any RMA data set will it be important to distinguish it (by adding the corresponding fields to the _prepare_merge_moves_distinct_fields() method) so that they aren't merged; otherwise, it won't matter if those fields aren't in the method, right?
There was a problem hiding this comment.
Not sure how the comparison is done. I'm just pointing possible failure reasons. And also because I don't think this will prevent access error. Have you tried without this if if it fails?
grindtildeath
left a comment
grindtildeath
left a comment
There was a problem hiding this comment.
Having a look here since I approved #613 .
IMO this is not right because _prepare_merge_moves_distinct_fields is an api.model function.
|
Conclusions:
The way to fix this in Another solution would be to override the |
|
Hello @victoralmau
Did you propose this change to Odoo? Although I agree it would be cleaner, IMO it's not significantly different that just applying I'd suggest to mention the Odoo PR as a comment in the fix of #613 so that we can merge it instead of keeping 2 open PRs on this repo trying to fix the same issue... What do you think? ping @pedrobaeza |
4 participants
Allow users without RMA permissions to confirm stock moves not related to RMA
@Tecnativa