Skip to content
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

[14.0][MIG] resource_booking: Migration to 14.0 #73

Merged
merged 39 commits into from
Jan 18, 2023

Conversation

pedrobaeza
Copy link
Member

  • Standard procedure
  • No more virtual ids
  • Adapt tests
  • FIX: Proper show_as meeting generation
  • IMP: Add name to tree view

@Tecnativa TT32344

Jairo Llopis and others added 30 commits October 23, 2022 12:10
This module adds a new app to allow you to book resource combinations in given
schedules.

Example use cases:

* Management of consultations in a clinic.
* Salesman appointments.
* Classroom and projector reservations.
* Hotel room booking.

Among the things you can do:

* Specify the type of booking, which includes a calendar of availability.
* Specify which resources can be booked together. All of them must be free to be booked.
* Place pending bookings, effectively giving permissions to someone to see the availability calendar and choose one slot.
* Partners can do that from their portals.
* If a partner has no user, he can still do the same via a tokenized URL.
* Backend users can also do that from the backend.
* Booking lifecycle with computed states.
* Automatic meeting creation and deletion.
* Automatic conflict detection.
* Deadline to block modifications.

@Tecnativa TT28201
Currently translated at 100.0% (190 of 190 strings)

Translation: calendar-12.0/calendar-12.0-resource_booking
Translate-URL: https://translation.odoo-community.org/projects/calendar-12-0/calendar-12-0-resource_booking/es/
…ating calendars

Without this patch, users couldn't change a calendar schedule if there were past or unconfirmed bookings that wouldn't fit in it.

Excluding those bookings from the check fixes the situation.

We also check that, to confirm a booking, it must fit in the calendar (because now it can happen that, in the time that has passed since the booking was scheduled until it is confirmed, the calendar changes).

@Tecnativa TT29509
The notifications emitted to the resource booking requester must always be in the same TZ as the resource booking itself.

For example, if you book one hotel room in the other side of the world, a notification in your own TZ is confusing.

Besides, res.partner created from website_sale are created with `tz=False`, making it even more confusing.

@Tecnativa TT30331
The constraint that checks the schedule of a resource booking is currently being applied to all the bookings, including past ones.
As the resource combination or associated calendars might change regularly and trigger a recomputation of this, such change might take a very long time.
Plus, the calendar restrictions might change, trigger a recompute of the constraint and detect bookings that can't be assigned, which makes no sense when they already happened.

This applies it only to future bookings, ignoring past ones.

TT30478
It's very unlikely that you need to book resources in a seconds-based precision.

This simplifies portal UI. Still, declared as a variable in case you need some customizations downstream.

@Tecnativa TT31063
- Standard v13 changes.
- Some onchanges removed in favor of computes.

@Tecnativa TT30987
From now on, `resource.booking.type` duration represents the default duration that will be applied to new `resource.booking` of that type, instead of the hardcoded duration for every new booking.

This means that you can create a new booking in pending state and define a custom duration before sending the portal link to customer.

Thus, the available schedule slots in portal will be based on the type duration, whereas only those that have enough time left to complete the booking duration will be displayed.

@Tecnativa TT30987
…cation

Thanks to this patch, you will be able to more predictably block the chosen resource combination, with the added checkbox.

Also, the location field is propagated to `resource.booking` records, and synced from there to `calendar.event` if needed. This way, you can alter the location before sending the portal invitation.

@Tecnativa TT30987
The filter wasn't working fine due to the `auto_join=True` set in `meeting_id`. It was never displaying pending bookings.

Besides, it wasn't necessary to keep it as a field because it was displayed nowhere and the search could be done directly in the view. This way, there's less python code to maintain and we disable the possibility of having a negative domain, which enters doomed scenarios when x2many fields are involved (see odoo/odoo#43957).

@Tecnativa TT30987
Adding `.with_context(exclude_public_holidays=True)` in needed places to let `hr_holidays_public` do its magic and never allow allocating a booking during a public holidays when that module is properly installed and configured.

@Tecnativa TT30987
- Allow users to optionally define a name for the resource booking.
- Display that name in portal if set.
- Sync it with meeting name when creating it.

@Tecnativa TT30987
Without this patch, when entering the Resource Bookings menu, if the current user attended any recurrent meeting, it failed with this error:

```
Server application error
 Error code: 200
 Error message: Odoo Server Error
 Error data message:
 invalid input syntax for integer: "82-20210726143000"
LINE 1: ... AND  ("resource_booking"."meeting_id" in (3,5,81,'82-202107...
                                                             ^

 Error data debug:
 Traceback (most recent call last):
  File "/opt/odoo/custom/src/odoo/odoo/http.py", line 624, in _handle_exception
    return super(JsonRequest, self)._handle_exception(exception)
  File "/opt/odoo/custom/src/odoo/odoo/http.py", line 310, in _handle_exception
    raise pycompat.reraise(type(exception), exception, sys.exc_info()[2])
  File "/opt/odoo/custom/src/odoo/odoo/tools/pycompat.py", line 14, in reraise
    raise value
  File "/opt/odoo/custom/src/odoo/odoo/http.py", line 669, in dispatch
    result = self._call_function(**self.params)
  File "/opt/odoo/custom/src/odoo/odoo/http.py", line 350, in _call_function
    return checked_call(self.db, *args, **kwargs)
  File "/opt/odoo/custom/src/odoo/odoo/service/model.py", line 94, in wrapper
    return f(dbname, *args, **kwargs)
  File "/opt/odoo/custom/src/odoo/odoo/http.py", line 339, in checked_call
    result = self.endpoint(*a, **kw)
  File "/opt/odoo/custom/src/odoo/odoo/http.py", line 915, in __call__
    return self.method(*args, **kw)
  File "/opt/odoo/custom/src/odoo/odoo/http.py", line 515, in response_wrap
    response = f(*args, **kw)
  File "/opt/odoo/auto/addons/web/controllers/main.py", line 1339, in call_kw
    return self._call_kw(model, method, args, kwargs)
  File "/opt/odoo/auto/addons/web/controllers/main.py", line 1331, in _call_kw
    return call_kw(request.env[model], method, args, kwargs)
  File "/opt/odoo/custom/src/odoo/odoo/api.py", line 383, in call_kw
    result = _call_kw_model(method, model, args, kwargs)
  File "/opt/odoo/custom/src/odoo/odoo/api.py", line 356, in _call_kw_model
    result = method(recs, *args, **kwargs)
  File "/opt/odoo/custom/src/odoo/odoo/models.py", line 4936, in search_read
    records = self.search(domain or [], offset=offset, limit=limit, order=order)
  File "/opt/odoo/custom/src/odoo/odoo/models.py", line 1648, in search
    res = self._search(args, offset=offset, limit=limit, order=order, count=count)
  File "/opt/odoo/custom/src/odoo/odoo/models.py", line 4499, in _search
    self._cr.execute(query_str, where_clause_params)
  File "/opt/odoo/custom/src/odoo/odoo/sql_db.py", line 173, in wrapper
    return f(self, *args, **kwargs)
  File "/opt/odoo/custom/src/odoo/odoo/sql_db.py", line 250, in execute
    res = self._obj.execute(query, params)
psycopg2.DataError: invalid input syntax for integer: "82-20210726143000"
LINE 1: ... AND  ("resource_booking"."meeting_id" in (3,5,81,'82-202107...
                                                             ^
```

@Tecnativa TT30987
Upstream ``unlink`` method in calendar.event adds this keyword argument.
When a user had no resource_booking permissions and opened a calendar event linked to a resource booking, he got an unnecessary access error.

@Tecnativa TT31238
When `_leave_intervals_batch()` in `resource.calendar` is extended by other modules that add more leaves for any reason, the returned recordset must be of the `resource.calendar.leaves` model. Otherwise, Odoo fails with:

    TypeError: '<' not supported between instances of 'resource.resource' and 'resource.calendar.leaves'

The resource didn't have a meaning here, so changing it to avoid bug.

@Tecnativa TT31249
Missing part from OCA#34:

- Display booking duration and location on portal, even on pending state.
- Improve portal test to contemplate new behaviors.

@Tecnativa TT31250
- If RBC (resource.booking.combination) was auto-assigned, attendees generated from RBC human resources are expected to confirm attendance manually.
- If RBC was handpicked, those attendees are auto-confirmed.
- All those confirmations are done before sending invitations, so invitations contain the correct attendance status.
- If requester books from portal view, he's also confirmed before sending invitation, for the same reason.

@Tecnativa TT31239
This allows better filtering while in the normal calendar view.

By default, the organizer will be whoever created the booking. Calendar invitations will go out in his name.

Also, avoid sending calendar event modification notifications if the modification isn't the schedule.

@Tecnativa TT31240
Includes those coming from requester and from resource combination.

@Tecnativa TT31901
Currently translated at 97.0% (196 of 202 strings)

Translation: calendar-13.0/calendar-13.0-resource_booking
Translate-URL: https://translation.odoo-community.org/projects/calendar-13-0/calendar-13-0-resource_booking/fr_FR/
Currently translated at 1.4% (3 of 202 strings)

Translation: calendar-13.0/calendar-13.0-resource_booking
Translate-URL: https://translation.odoo-community.org/projects/calendar-13-0/calendar-13-0-resource_booking/fr/
When some partner is a resource assigned to a booking, he should get auto-subscribed.

Inspired in OCA#42, but I'm changing that behavior.

@Tecnativa TT32148
By using the upstream `mail.message_user_assigned` template, we were getting this exception:

```
odoo.addons.base.models.qweb.QWebException: 'resource.booking' object has no attribute 'user_id'
Traceback (most recent call last):
  File "/opt/odoo/custom/src/odoo/odoo/addons/base/models/qweb.py", line 348, in _compiled_fn
    return compiled(self, append, new, options, log)
  File "<template>", line 1, in template_2601_10519
AttributeError: 'resource.booking' object has no attribute 'user_id'
```

So I provide a specific template for this kind of subscription notification, which doesn't use that `user_id` field. Besides, it explains better why you're getting it.

@Tecnativa TT32190
Forward-port of OCA#51.

Includes a modification for the migration script of 13.0.1.0.0 (if you migrate from v12, you'll need it; if you migrate from v13, it will be automatic).

@Tecnativa TT31901
Without this patch, if you create a resource booking for a type that has categories (tags), it will fail with `CacheMiss` error.

@Tecnativa TT32552
Jairo Llopis and others added 9 commits October 23, 2022 12:18
If I'm creating bookings for my colleagues, I'll be the owner of all of the meetings. But that doesn't mean I'll be busy.

With this patch, only when I'm a booked resource will the event be set as busy.

@Tecnativa TT32666
Apply lessons from https://youtu.be/lT___hG110Q?t=23285 and disable unnecessary mail stuff.

Before:

    odoo.addons.resource_booking.tests.test_backend tested in 16.18s, 9124 queries
    odoo.addons.resource_booking.tests.test_portal tested in 16.42s, 6685 queries

After:

    odoo.addons.resource_booking.tests.test_backend tested in 13.68s, 7873 queries
    odoo.addons.resource_booking.tests.test_portal tested in 15.17s, 5972 queries

@Tecnativa TT31324
Public users usually don't have an email, so they hit [this error][1].

No test added because anyways the tests in `website_sale_resource_booking` already fail without this patch, and it's hard to find a use case without it.

@Tecnativa TT31328

[1]: https://github.com/odoo/odoo/blob/e7dd314e53a4260dd1494cbcb7dcef1c407e2a8a/addons/mail/models/mail_message.py#L36
We need a stop parameter. Otherwise, the calendar view could be trying
to render tons of records making the client's browser have a hard time.

TT35062
`rrule` isn't very performant on iterations. This is used on calendar
events to compute the virtual events set.

We want to avoid calling it too much.

Real case with website_sale_resource_booking (thousands of virtual events):

before: /shop/booking/<id>/schedule: ~25 seconds
after: /shop/booking/<id>/schedule: ~6 seconds

before: /shop/booking/<id>/confirm: ~20 seconds
after: /shop/booking/<id>/confirm: ~3 seconds

before: /shop/checkout: ~18 seconds
after: /shop/checkout: ~2 seconds

There's room for improvement anyway
Restrict the events which our resource is participating in to avoid
loading recurrent events as much as possible. This is really more
performant in v13. A simple showcase:

>>> timeit(
        lambda: env["calendar.event"].with_context(active_test=True).search(
            [("partner_ids", "=", partner.id)]
        ), number=1
    )
0.06541907804785296
>>> timeit(
        lambda: env["calendar.event"].with_context(active_test=True).search([])
        , number=1
    )
1.194712486001663

TT37089
- Standard procedure
- No more virtual ids
- Adapt tests
- FIX: Proper show_as meeting generation
- IMP: Add name to tree view

TT32344
@pedrobaeza
Copy link
Member Author

/ocabot migration resource_booking

@OCA-git-bot OCA-git-bot added this to the 14.0 milestone Oct 24, 2022
@OCA-git-bot OCA-git-bot mentioned this pull request Oct 24, 2022
1 task
@pedrobaeza
Copy link
Member Author

@njeudy ready to be reviewed

Comment on lines 12 to 13
"maintainers": ["Yajo"],
"maintainers": ["pedrobaeza"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking maintainership of this module. ❤️ In the future I'll appreciate it if you ping me about these kind of changes instead of just doing them without notice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you are not anymore with us, so it's logic that you are not going to maintain this module. But if you prefer to keep maintaining it, I'm glad to leave it to you. But you can't retain something that you are not going to maintain, and you haven't expressed such desire nor do anything in this module since your departure, so I don't feel in the obligation of warning your for each module you have done under Tecnativa.

Comment on lines 307 to 305
# If you're not booked, you're free
show_as=(
"busy"
if self.env.user.partner_id in resource_partners
else "free"
),
show_as="busy",
Copy link
Member

@huguesdk huguesdk Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this change removed here (and the test for the show_as property value in test_backend.BackendCase.test_event_show_as_free()), while it was added recently in 13.0? if the lines checking for the value are re-added to the test, it fails.

edit: it was not added recently to 13.0, this was exactly one year ago. ☺️ but the question remains: why remove it in 14.0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this was a misconception: the meeting should always be busy, as the current user shouldn't influence on the state. Do you have any use case about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, no use case, only wondering about the reason of the change. thanks for the explanation!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The misconception is that one: other colleague creating resource bookings is not going to be on the attendee list if they simply aren't in the resource combination list nor organizer.

@gurneyalex
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-73-by-gurneyalex-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jan 18, 2023
Signed-off-by gurneyalex
@OCA-git-bot
Copy link
Contributor

It looks like something changed on 14.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 14.0-ocabot-merge-pr-73-by-gurneyalex-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 9954ee6 into OCA:14.0 Jan 18, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 06dcd40. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants