New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[15.0][FIX] calendar: Select correct base event when current is archived #149349
Conversation
f441349
to
8f7b3a1
Compare
Isn't this reproducible when you add the recurrence from a non-included day? Or do you need to manually archive the base event? |
Yes sorry, when a recurring calendar event is created from a day that is not included in the recurrence, it is archived and the recurrence data in that event is lost, but it remains the base event of the recurrence. Therefore, when trying to make changes to the recurrence, the following error is thrown:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And please change the steps to reproduce the problem (both in the main PR message and in the commit message) with the real use case.
8f7b3a1
to
d8b8847
Compare
d8b8847
to
744cac9
Compare
Changes done @pedrobaeza |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, thanks for these changes. Could you add a small test to make sure the issue is not coming back ? It will improve our codebase. Thanks a lot. :-)
744cac9
to
1d4f89e
Compare
@arj-odoo test added 😄 |
class TestUpdateMultiDayWeeklyDayNotIncludedRecurrentEvents(TestRecurrentEvents): | ||
@classmethod | ||
def setUpClass(cls): | ||
super().setUpClass() | ||
cls.event = cls.env['calendar.event'].create({ | ||
'name': 'Recurrent Event', | ||
'start': datetime(2019, 10, 22, 1, 0), | ||
'stop': datetime(2019, 10, 22, 2, 0), | ||
'recurrency': True, | ||
'rrule_type': 'weekly', | ||
'tue': False, | ||
'wed': True, | ||
'fri': True, | ||
'interval': 1, | ||
'count': 3, | ||
'event_tz': 'Etc/GMT-4', | ||
}) | ||
cls.recurrence = cls.env['calendar.recurrence'].search([]) | ||
cls.events = cls.recurrence.calendar_event_ids.sorted('start') | ||
# Tuesday datetime(2019, 10, 22, 1, 0) | ||
# Wednesday datetime(2019, 10, 23, 1, 0) | ||
# Friday datetime(2019, 10, 25, 1, 0) | ||
# Wednesday datetime(2019, 10, 30, 1, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you adding a new test class ? The setup will be heavy for just one single test. You can do everything in the current class. Your test coould be in the class TestUpdateRecurrentEvents. Thanks :-)
if values.get("active") is False: | ||
recurrences = self.env["calendar.recurrence"].search([ | ||
('base_event_id', 'in', self.ids) | ||
]) | ||
recurrences._select_new_base_event() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your change is working but I am afraid of the performances with a large amount of events. I hope it won't be too slow and rollback 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this PR I have used the same philosophy that has been used for the unlink method. So I understand that there will be the same doubt for that method…
odoo/addons/calendar/models/calendar_event.py
Lines 585 to 592 in 7778644
recurrences = self.env["calendar.recurrence"].search([ | |
('base_event_id.id', 'in', [e.id for e in self]) | |
]) | |
result = super().unlink() | |
if recurrences: | |
recurrences._select_new_base_event() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, the scope is limited to when you archive an event, so the impact is also very limited.
1d4f89e
to
c134a8a
Compare
@robodoo r+ |
@CarlosRoca13 @arj-odoo staging failed: ci/runbot on 2cccbdeb2f28e3439cc0669109012d8d471f8b70 (view more at https://runbot.odoo.com/runbot/build/56671293) |
Something has failed when trying to set this changes @arj-odoo |
Can you rebase the code? I lost this one in the process, sorry /o\ |
c134a8a
to
20d96b5
Compare
When the base event is archived, it continues being the base event of the recurrence, and when trying to change the recurrence of all the events of the recurrence, an error is thrown or some inconsistencies occur. To test the problem, you can follow these steps: 1. Create a recurrence of events from a non included day on the recurrence (example: recurrence on tuesday and friday and the start of the recurrence on monday). The first event will be archived automatically. 2. Open other event of the recurrence. 3. Modify the recurrence for all events (example: change the weekdays, set just tuesday instead of tuesday and friday) An error will be thrown. By making these changes, the base event will be updated, as indicated in the _select_new_base_event method, so these inconsistencies will not occur when making the changes.
20d96b5
to
34873de
Compare
Done @arj-odoo |
@robodoo r+ |
When the base event is archived, it continues being the base event of the recurrence, and when trying to change the recurrence of all the events of the recurrence, an error is thrown or some inconsistencies occur. To test the problem, you can follow these steps: 1. Create a recurrence of events from a non included day on the recurrence (example: recurrence on tuesday and friday and the start of the recurrence on monday). The first event will be archived automatically. 2. Open other event of the recurrence. 3. Modify the recurrence for all events (example: change the weekdays, set just tuesday instead of tuesday and friday) An error will be thrown. By making these changes, the base event will be updated, as indicated in the _select_new_base_event method, so these inconsistencies will not occur when making the changes. closes #149349 Signed-off-by: Arnaud Joset (arj) <arj@odoo.com>
When the base event is archived, it continues being the base event of the recurrence, and when trying to change the recurrence of all the events of the recurrence, an error is thrown or some inconsistencies occur.
To test the problem, you can follow these steps:
An error will be thrown.
See next gif:
By making these changes, the base event will be updated, as indicated in the
_select_new_base_event
method, so these inconsistencies will not occur when making the changes.See next gif:
cc @Tecnativa TT46742
ping @pedrobaeza @chienandalu
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr