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

[16.0][MIG] resource_booking: Migration to 16.0 #101

Closed
wants to merge 61 commits into from

Conversation

norlinhenrik
Copy link

@norlinhenrik norlinhenrik commented Aug 30, 2023

Improvements in this migration:

  • Booking: Refactored _get_available_slots() works also when each slot is multiple days
  • Booking Type: new field slot_duration
  • Contact: button -> Bookings
  • Combination -> Bookings -> Create booking: Set the default combination from the navigation
  • Booking: may have multiple contacts
  • Booking: When type_id is missing (booking from Timeline view), get empty Intervals (instead of error)
  • Booking: Search on combination
  • Booking: Refactored _availability_is_fitting()
  • Booking: List view with hidden contacts (the new field)
  • Work intervals (with migration): To span multiple days, set hour_to = 24 instead of 23:59
  • XML renaming (with migration)
  • [15.0][IMP] resource_booking: Allow a booking to span more than one calendar day #100

Later PRs for version 15.0 of resource_booking (104, 106, 110, 111) are not included in this PR.

@norlinhenrik
Copy link
Author

image
The partner calendar dropdown menu does not work in version 16.0.
I am not a frontend developer. Is there anyone else who could troubleshoot this?

@norlinhenrik
Copy link
Author

I had to rename data-toggle to data-bs-toggle. I did the same with some other data attributes, in a separate commit.

@pedrobaeza
Copy link
Member

Check OCA/openupgradelib#338

@norlinhenrik
Copy link
Author

Thank you for the link, @pedrobaeza
How do I use the BS4 to BS5 tool to update a module?

data-target should change to data-bs-target.
Should t-attf-data-target change to t-attf-data-bs-target ?

@pedrobaeza
Copy link
Member

How do I use the BS4 to BS5 tool to update a module?

It's more like a mapping reference than a direct tool

Should t-attf-data-target change to t-attf-data-bs-target ?

Yes, as the prefix t-attf- means that the rest of the attribute is parametric.

@norlinhenrik norlinhenrik force-pushed the 16.0-mig-resource_booking branch 2 times, most recently from 8f63084 to 1606216 Compare September 19, 2023 18:08
@norlinhenrik
Copy link
Author

I have included Allow a booking to span more than one calendar day #100

@yajo @pedrobaeza Would you like to review?

@victoralmau
Copy link
Member

Please, cherry-pick #104 (resolves the comment in #101 (comment))

@loymcom loymcom force-pushed the 16.0-mig-resource_booking branch 3 times, most recently from c225a57 to 7921fe3 Compare October 5, 2023 07:02
Copy link

@stefan-tecnativa stefan-tecnativa left a comment

Choose a reason for hiding this comment

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

Please, solve the web_calendar_slot_duration dependency and attend the comments.

@Tecnativa TT45649

default_type_id=self.id,
# Context used by web_calendar_slot_duration module
calendar_slot_duration=FloatTimeParser.value_to_html(
self.duration, False

Choose a reason for hiding this comment

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

Suggested change
self.duration, False
self.slot_duration, False

You added a slot_duration field intentionally to separe the duration of the booking and the slot in calendar, so this change is consequent. But anyway this change is uselles because without the dependency of web_calendar_slot_duration this doesn't work.
If you look at my PR: OCA/web#2646 , you must notice that the slot_duration that expects is in this format: (00:00:00), but this FloatTimeParser function returns it in this one: (00:00). So you will have to make a conversion from the Float to this first format.

Copy link

Choose a reason for hiding this comment

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

Is (00:00:00) new for v16?
If yes, can you help me with this conversion?
If no, can you show me where the conversion is done for self.duration?

@@ -25,7 +25,6 @@
"mail",
"portal",
"resource",
"web_calendar_slot_duration",

Choose a reason for hiding this comment

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

You can't remove this dependency, it is necessary to maintain the functionallity of the module. Here I post a PR that i made migrating correctly the module: OCA/web#2646

@carolinafernandez-tecnativa

Hi how are you? There is one commit missing from 15.0
a4548a1

Copy link
Member

@flotho flotho left a comment

Choose a reason for hiding this comment

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

Thanks for this migration.
Functional tests in progress
ACL issue for admin :
image

IMHO, admin user should be able to setup thoses exceptions

@flotho
Copy link
Member

flotho commented Oct 30, 2023

Question regarding the process,
Does this module could also replace something like calendly ?
In this case, how to share available slot to be booked on the portal and/or on public website ?
Regards

@pedrobaeza
Copy link
Member

Yes, it can act like Calendly. Just click on the Share button of the resource booking.

@norlinhenrik
Copy link
Author

Thank you for all the feedback. I will work on this one of the next days.

@norlinhenrik
Copy link
Author

Thanks for this migration. Functional tests in progress ACL issue for admin : image

IMHO, admin user should be able to setup thoses exceptions

Security for Resource Time Off is defined in resource. Admin may create global leaves, but not leaves for specific human resources.

@norlinhenrik
Copy link
Author

Should I squash together all my commits, or just those who are highly related?

Improvements in this migration:

  • Booking: Refactored _get_available_slots() works also when each slot is multiple days
  • Booking Type: new field slot_duration
  • Contact: button -> Bookings
  • Combination -> Bookings -> Create booking: Set the default combination from the navigation
  • Booking: may have multiple contacts
  • Booking: When type_id is missing (booking from Timeline view), get empty Intervals (instead of error)
  • Booking: Search on combination
  • Booking: Refactored _availability_is_fitting()
  • Booking: List view with hidden contacts (the new field)
  • Work intervals (with migration): To span multiple days, set hour_to = 24 instead of 23:59
  • XML renaming (with migration)

@JavierIniesta
Copy link

Hi @norlinhenrik

Thank you for your efforts.

Could you push again to see if the problem with the pre-commit is resolved? It looks like there was some problem during the process with the nodejs environment.

Jairo Llopis and others added 6 commits November 27, 2023 21:06
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
@norlinhenrik
Copy link
Author

@pedrobaeza CONTRIBUTING mentions oca_dependencies.txt which seems apropiate in this case too because web_calendar_slot_duration dependency. Is it an outdated convention???

I would like to update the documentation about oca_dependencies.txt. Please join the discussion here.

@rrebollo
Copy link

rrebollo commented Jan 31, 2024

@norlinhenrik I was doing some testing using runboat build on the last commit to this PR. Something bizarre it's happening using a tokenized url to portal to schedule the book: It gets stuck on December 2023. See what I mean. First I'm showing the settings (Basically everyone is in the same TZ), later the "problem". I'm using a negative timezone (-5 I think):

Peek Portal Schedule booking stuck on same month

Surely the core of the issue is:
here

    def _get_calendar_context(self, year=None, month=None, now=None):
        """Get the required context for the calendar view in the portal.


        See the `resource_booking.scheduling_calendar` view.


        :param int year: Year of the calendar to be displayed.
        :param int month: Month of the calendar to be displayed.
        :param datetime now: Represents the current datetime.
        """
        month1 = relativedelta(months=1)
        now = now or fields.Datetime.now()
        year = year or now.year
        month = month or now.month
        start = datetime(year, month, 1)
        start, now = (
            fields.Datetime.context_timestamp(self, dt) for dt in (start, now)
        )
        start = start.replace(hour=0, minute=0, second=0, microsecond=0)
        lang = self.env["res.lang"]._lang_get(self.env.lang or self.env.user.lang)
        weekday_names = dict(lang.fields_get(["week_start"])["week_start"]["selection"])
        booking_duration = timedelta(hours=self.duration)
        slots = self._get_available_slots(start, start + month1 + booking_duration)
        return {
            "booking": self,
            "calendar": calendar.Calendar(int(lang.week_start) - 1),
            "now": now,
            "res_lang": lang,
            "slots": slots,
            "start": start,
            "weekday_names": weekday_names,
        }

It isn't clear to me why:

start, now = (
            fields.Datetime.context_timestamp(self, dt) for dt in (start, now)
        )

should'nt the target here be January 1st, 2024 no matter TZ? @pedrobaeza Am I getting things wrong?
The "issue" happens to be "solved" with

❯ git diff
diff --git a/resource_booking/models/resource_booking.py b/resource_booking/models/resource_booking.py
index effe7d9..ccea10d 100644
--- a/resource_booking/models/resource_booking.py
+++ b/resource_booking/models/resource_booking.py
@@ -508,7 +508,7 @@ class ResourceBooking(models.Model):
             "now": now,
             "res_lang": lang,
             "slots": slots,
-            "start": start,
+            "start": fields.Date.from_string(f'{year}-{month}-01'),
             "weekday_names": weekday_names,
         }
 
diff --git a/resource_booking/templates/portal.xml b/resource_booking/templates/portal.xml
index 8931d7e..54b82fd 100644
--- a/resource_booking/templates/portal.xml
+++ b/resource_booking/templates/portal.xml
@@ -44,7 +44,7 @@
                     <tr>
                         <th class="text-left">
                             <a
-                                t-if="start > now"
+                                t-if="start > now.date()"
                                 t-att-href="booking.get_portal_url(suffix='/schedule/%d/%d' % (start_previous.year, start_previous.month))"
                                 class="btn btn-secondary"
                                 title="Previous month"

@norlinhenrik
Copy link
Author

start should be the very beginning of the month, in the timezone of self.env["tz"].
This is fixed in the latest commit.

@rrebollo
Copy link

rrebollo commented Feb 1, 2024

@norlinhenrik partner auto-subcription to bookings looks like isn't working either here. I'm taking this.
Edited: auto subscription doesn't work on booking creation. It's fine at update.

@rrebollo
Copy link

rrebollo commented Feb 1, 2024

@norlinhenrik partner auto-subcription to bookings looks like isn't working either here. I'm taking this. Edited: auto subscription doesn't work on booking creation. It's fine at update.

@norlinhenrik I think it is intended because in related calendar events followers are rigthtly settled. So I think no work is required here.

@rrebollo
Copy link

rrebollo commented Feb 1, 2024

@norlinhenrik I made an small fix. When trying to schedule a booking from booking form view but no name has been given to the booking it raises an OWl error when clicking on the calendar view. I created a PR to your branch. Please check it out.

@ayushin
Copy link

ayushin commented Feb 3, 2024

not to rush anybody, but lets keep the momentum!

i'm willing to help in any way i can to get this merged asap

@norlinhenrik
Copy link
Author

@ayushin If you have tested and think it is ready to merge, then click on Files changed -> Review changes -> Approve.

Copy link

@jelenapoblet jelenapoblet left a comment

Choose a reason for hiding this comment

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

It's working just fine, i've been testing it out on runboat. The only thing that bugs me a little it's on the smartphone view.
WhatsApp Image 2024-02-05 at 10 32 45 PM

@rrebollo
Copy link

rrebollo commented Feb 6, 2024

It's working just fine, i've been testing it out on runboat. The only thing that bugs me a little it's on the smartphone view. WhatsApp Image 2024-02-05 at 10 32 45 PM

It's happening here too. Any known frontend dev to assit us???

@rrebollo
Copy link

rrebollo commented Feb 6, 2024

@jelenapoblet I think I "fixed" the calendar going wider. According to Bootstrap 5.1 doc .table-responsive-md should be in a div as parent for the table element with .table. @norlinhenrik PR loymcom#1.

@pedrobaeza
Copy link
Member

/ocabot migration resource_booking

Please finish your reviews.

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Feb 7, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Feb 7, 2024
1 task
@norlinhenrik
Copy link
Author

@rrebollo I have included your latest commit.

@pedrobaeza
Copy link
Member

Great, please squash all the commits by author into their migration one for being mergeable.

@norlinhenrik
Copy link
Author

Done

"python": [
# Used implicitly
"cssselect",
"freezegun",
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct. This dependency is not for the module, but for tests. You must remove it from here and put it in test_requirements.txt file in root.

Choose a reason for hiding this comment

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

@norlinhenrik I have pushed to loymcom#1 a commit fixing this

Copy link
Member

Choose a reason for hiding this comment

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

cssselect may be needed for the module itself. I was talking about freezegun. Anyway, the CI is still red.

norlinhenrik and others added 2 commits February 28, 2024 20:46
[IMP] resource_booking: new field slot_duration

[IMP] resource_booking: Button (partner -> booking)

[IMP] resource_booking: combination -> bookings -> create: default combination

[IMP] resource_booking: A booking may have multiple contacts, e.g. people in a room

[FIX] resource_booking: _get_intervals() when type_id is missing

[IMP] resource_booking: booking: search on combination

[FIX] resource_booking: _availability_is_fitting()

[IMP] resource_booking: booking list view with hidden partner_ids

[FIX] resource_booking: migration: attendance hour_to 23:59 -> 24:00

[IMP] resource_booking: rename_xmlids

[FIX] resource_booking: pre-commit

[FIX] resource_booking: calendar_slot_duration format

[FIX] resource_booking: _get_calendar_context() with correct start / timezone
[IMP] resource_booking: fix failling BackendCase. test_booking_from_calendar_view

[IMP] resource_booking: fix typo in help parameter for combination_assignment field from resource_booking_type

[IMP] resource_booking: fix failing PortalCase.test_portal_no_bookings

Test was failling cause there was no bookings link on portal home for external users, then no trigger.
Also I had to tweak eslintrc config cause precommit was failling due to ECMA version.

[IMP] resource_booking: No create/unlink call if nothing to do

[FIX] resource_booking: error CalendarQuickCreate title is not a string when scheduling a booking

[FIX] resource_booking: portal responsive booking calendar table going wider and no x scroll
@norlinhenrik
Copy link
Author

All checks have passed now.

)
name = fields.Char(index=True, help="Leave empty to autogenerate a booking name.")
description = fields.Html()
partner_id = fields.Many2one(
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct according v15:

https://github.com/OCA/calendar/blob/15.0/resource_booking/models/resource_booking.py#L126

The compute/inverse is in this field, not in partner_ids.

Copy link
Author

@norlinhenrik norlinhenrik Mar 3, 2024

Choose a reason for hiding this comment

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

partner_ids was added to 15.0 after I suggested it for 16.0. Probably I don't need this field after all.

@pedrobaeza Feel free to propose a new commit to do the correction. You may remove partner_ids if you want.

Copy link
Author

Choose a reason for hiding this comment

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

But I think the field may be practcal in some scenarios, even though I don't need it right now.

Copy link
Member

Choose a reason for hiding this comment

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

@victoralmau please resume this migration including all the 15.0 stuff. Please complete and include #114 as well.

@victoralmau
Copy link
Member

Superseed by #120

@pedrobaeza
Copy link
Member

As the other has preserved attribution, closing in favor of it.

@pedrobaeza pedrobaeza closed this Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet