[FIX] contract_line_successor: always call _prepare_value_for_stop() …

[bjouini-acsone]

…on stop()

Before, when the date_end we want to set was not smaller than the line's current date_end, stop did not call _prepare_value_for_stop. It only wrote is_auto_renew and manual_renew_needed by hand instead.

This is a problem because other modules can override _prepare_value_for_stop() to add their own logic If stop() does not call this method, all that logic is skipped and their values are never saved.

Now _prepare_value_for_stop is always called. We still never make date_end bigger than what it already is we just make sure the method is called in all cases so custom values from other modules are not lost.

[OCA-git-bot]

Hi @sbejaoui,
some modules you are maintaining are being modified, check this out!

[qgroulard]

Hi, thanks for the PR.
I agree that we would gain in extensibility by always calling _prepare_value_for_stop. I believe the reasons for not calling it in the first place are not valid anymore (it doesn't arm anymore to write the date_end if it doesn't change).

However I wouldn't expect the call to _prepare_value_for_stop to change, see my comment.

[qgroulard]

I am confused, why does the call to _prepare_value_for_stop() has to change and be called with effective_date_end ?

[bjouini-acsone]

we change it because _prepare_value_for_stop just take the date_end we give it and use it, it does not check anything but stop has a rule ,it should never make date_end bigger than what it already is
before, this rule was done by skipping _prepare_value_for_stop when date_end was not smaller
so now we always call _prepare_value_for_stop, but to keep the rule, we compute effective_date_end first and give that instead of the raw date_end. Like this the rule is still respected, and all overrides still go correctly always

[qgroulard]

Ok then maybe the cleaner fix is to keep the current structure but take date_end out of _prepare_value_for_stop.

We could even have another method for the actual earlier stop which calls _prepare_value_for_stop and add date_end to the dict.
This way, we have two possible extension points: one generic and one for the case where we stop before the end date.

[bjouini-acsone]

@qgroulard you can check this version

[qgroulard]

Yes I like it better but I wouldn't name this method "with_date_end" since the goal is to make it extensible in order to add more potential fields to update.

Btw, recurring_next_date is also part of this flow only. Although I am not sure we still need to update this field since it is automatically recomputed.

[bjouini-acsone]

changed the naming to _prepare_common_value_for_stop, is that okay ?

about the recurring_next_date, for me we should not remove this , it's okay that it's a computed field but also it's not a Readonly field , so for me the reason is that when we stop the line we should recalculate the value of the field even if the user have set the alue manually before (not sure if that's the reason of this implementation or not )

[qgroulard]

I agree to not touch recurring_next_date, however:

1. recurring_next_date should be updated in the "update date end" workflow, not the other (in order to keep the logic as before).
2. The "update date end" workflow should call the method named _prepare_value_for_stop in order to keep backward compatibility. Your new method should be the base one, and not the other way around.