[FIX] beesdoo_shift_swap: Prevent a skip of subsequent items

[carmenbianca]

Description

Modifying a list while looping over it is not allowed. It causes subsequent items to be skipped.

I suspect that this is the cause of a bug where a person who wants to swap a shift is correctly unsubscribed from their old shift, but not subscribed to their new shift. But I have not been able to verify this.

It is not clear to me why this code exists.

Odoo task (if applicable)

T12851

Checklist before approval

• Tests are present (or not needed).
• Credits/copyright have been changed correctly.
• Change log snippet is present.
• (If a new module) Moving this to OCA has been considered.

[polchampion]

Successful functional test on fhmunich-test: no side-effect detected

[polchampion]

@remytms can you review?

[huguesdk]

this change seems correct to me, even though i don’t know much about the code.

[huguesdk]

/ocabot merge patch

[github-grap-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 12.0-ocabot-merge-pr-534-by-huguesdk-bump-patch, awaiting test results.

[github-grap-bot]

@huguesdk your merge command was aborted due to failed check(s), which you can inspect on this commit of 12.0-ocabot-merge-pr-534-by-huguesdk-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

[huguesdk]

@carmenbianca some tests are failing but i don’t know whether it’s because of the change. would you please check this?

[carmenbianca]

@huguesdk fixed the test

[huguesdk]

@carmenbianca while trying to understand the code, i changed it a little to make it more obvious. what do you think?

[carmenbianca]

Your proposal is not at all easy to ready for me, sorry :(

I think my code is easier to read, although it does require a comment for clarity. I do like one change you made, though. I think we should rename changes to to_process.

[huguesdk]

with the previous code, i couldn’t understand at first why elements had to be removed from the list, and thus, why the list which is iterated over must be a copy. i think it was confusing for you too at first, which is probably why you simply removed the code removing the elements.

it is only after debugging the tests that i understood that changes is used across iterations on shifts, which is why it had to be updated to contain only the elements that still need to be processed.

what bothers me with the previous code is the use of list.remove(), which will have to search for the element in the list, while we’re already iterating over the elements. that’s why i think it makes more sense to construct a new list with all not-yet-processed elements along the way. what do you think about this? how would you make that code more clear?

[carmenbianca]

@huguesdk I gave it another go, using sets. I added a rationale to the commit message.

[huguesdk]

i’m sorry but the part of my brain that likes performance hurts when reading this, because of 2 reasons:

1. the set is sorted in each iteration on the shifts instead of just once.
2. the set of processed changes are removed from the set, which is correct but much less performant (in a theoretical way) than just building a new list along the way.

sets are cool to ensure elements are there only once, but to iterate over a collection in a certain order, lists are better.

i’ve searched for recommended ways of removing elements of a list while iterating over it, and the answers suggest to use a list comprehension and either create a new list, or assign the new elements to a slice of the list. i added a commit doing that. what do you think?

[carmenbianca]

@huguesdk I like this proposal. One comment. Good to merge also if you disagree with my comment.

[carmenbianca]

It is not clear to me if [:] is needed. Can't you do a list comprehension instead of a generator? i.e. replace (rec for rec in ...) with [rec for rec in ...]

[huguesdk]

you’re right. this makes sense only if the list is really big and making a copy of it should be avoided. i’ve changed it.

[huguesdk]

/ocabot merge patch

[github-grap-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 12.0-ocabot-merge-pr-534-by-huguesdk-bump-patch, awaiting test results.

[github-grap-bot]

Congratulations, your PR was merged at c894f16. Thanks a lot for contributing to beescoop. ❤️