Link booking occurrences to events #6243

moliholy · 2024-03-20T16:37:00Z

This PR links booking occurrences with events.

Screenshots

indico/modules/rb/models/reservation_occurrences.py

indico/modules/rb/controllers/backend/bookings.py

indico/modules/rb/schemas.py

indico/modules/rb/templates/booking_list.html

moliholy · 2024-03-26T13:40:43Z

@ThiefMaster this is ready for round 2.

ThiefMaster · 2024-03-26T13:42:46Z

I'm busy preparing the 3.3 release atm, but maybe one of my colleagues can give it a try in the meantime :)

tomasr8 · 2024-03-26T14:48:48Z

I'm getting this error when linking a booking:

The booking still gets linked though 🤔

indico/modules/rb/client/js/modules/calendar/CalendarListView.jsx

moliholy · 2024-03-30T14:50:44Z

I'm getting this error when linking a booking:

@tomasr8 it's fixed now. It was redux stuff.

tomasr8 · 2024-04-02T12:37:56Z

indico/modules/rb/client/js/modules/calendar/CalendarListView.jsx

+        />
+      );
+      return (
+        <div style={{position: 'absolute', top: '5px', right: '5px'}}>


With longer room names, there's an overlap between them and the link button:

I moved it a bit down so that it does not overlap.

indico/modules/rb/client/js/modules/calendar/CalendarListView.jsx

tomasr8 · 2024-04-02T12:41:37Z

indico/modules/rb/client/js/modules/calendar/CalendarListView.jsx

+        <Button
+          icon={<Icon name="linkify" />}
+          primary
+          disabled={!!booking.link || !datesMatch}


What about not rendering the link button at all if it's not possible to link? If you have a lot of bookings, it could be hard to see the difference between active and disabled buttons since the shades of blue are pretty similar.

I just removed rendering the button if it cannot be linked.

tomasr8 · 2024-04-02T12:50:53Z

Should it be possible to unlink the booking as well? Since you can link with just a click w/o a confirmation I can imagine someone accidentally selecting the wrong booking. Or just add a confirmation dialog when selecting the booking?

indico/modules/rb/models/reservation_occurrences.py

moliholy · 2024-04-05T07:57:46Z

Should it be possible to unlink the booking as well? Since you can link with just a click w/o a confirmation I can imagine someone accidentally selecting the wrong booking. Or just add a confirmation dialog when selecting the booking?

I just added a modal to confirm the linking operation.

ThiefMaster · 2024-05-02T16:14:21Z

I made a few improvements while testing it and it's almost good to go, unfortunately there is still a rather nasty bug - I'll comment on the code about it since that way it'll be a separate comment thread.

indico/modules/rb/controllers/backend/bookings.py

indico/modules/rb/client/js/modules/calendar/CalendarListView.jsx

Also use a tuple instead of a list in overlaps check

`check_access` is meant for access checks, not for general validation

OmeGak · 2024-05-16T16:31:37Z

Adding a short video demo:

Screen.Recording.2024-05-16.at.18.21.37.mov

ThiefMaster · 2024-05-31T12:08:22Z

indico/modules/rb/client/js/modules/calendar/CalendarListView.jsx

+    const boundaries = [moment(linkData.startDt), moment(linkData.endDt)];
+    const datesMatch =
+      moment(booking.startDt).isBetween(...boundaries, undefined, '[]') &&
+      moment(booking.endDt).isBetween(...boundaries, undefined, '[]');


I think this check may be too strict. IMHO it's reasonable to allow linking a booking that "contains" the event - e.g. when the booking is from 8:30-11:00 and the event is from 9:00-10:30.

And isn't that the case with the current code? I mean, if both the events's start and end dates are between the booking's timeframe, then the event's timeframe is inside the booking's, right?

EDIT: is it just me or... the current code is exactly the other way around? 🤔

Nope, the current logic requires that both conditions are met:

the occurrence start is between the start/end date of the event

the occurrence end is between the start/end date of the event

However, I think what we need is a check that:

the occurrence start is <= the event start

the occurrence end is >= the event end

Or maybe even have a general "time ranges overlap" check since you may want to link multiple occurrences that are used for the event even though neither of them fully contains the event?

That would be (StartOccurrence < EndEvent) and (EndOccurrence > StartEvent)

That does not work. Imagine the following:

Event: 10::00 - 12:00.
Occurrence: 11:00 - 16:00.

Using your formula:
11:00 < 12:00 and 16:00 > 10:00

Yet between 10:00 and 11:00 there's an event but there's no booking.

moliholy force-pushed the booking-occurrence-linking branch 4 times, most recently from 34c3061 to 42f6955 Compare March 21, 2024 11:16

moliholy marked this pull request as ready for review March 21, 2024 11:17

moliholy force-pushed the booking-occurrence-linking branch 4 times, most recently from 8f38100 to f84165a Compare March 21, 2024 11:47

moliholy changed the title ~~link booking occurrencies to events~~ link booking occurrences to events Mar 21, 2024

moliholy changed the title ~~link booking occurrences to events~~ Link booking occurrences to events Mar 21, 2024

moliholy force-pushed the booking-occurrence-linking branch 2 times, most recently from 0b93dc2 to e61c064 Compare March 21, 2024 12:59

ThiefMaster reviewed Mar 21, 2024

View reviewed changes

indico/modules/rb/templates/booking_list.html Outdated Show resolved Hide resolved

moliholy force-pushed the booking-occurrence-linking branch from 1706604 to bd1852d Compare March 25, 2024 10:59

ThiefMaster requested a review from tomasr8 March 26, 2024 13:42

tomasr8 reviewed Mar 26, 2024

View reviewed changes

indico/modules/rb/client/js/modules/calendar/CalendarListView.jsx Outdated Show resolved Hide resolved

ThiefMaster force-pushed the master branch from 552297c to a46dd5e Compare March 27, 2024 10:27

moliholy force-pushed the booking-occurrence-linking branch 2 times, most recently from d400643 to 98eba97 Compare March 30, 2024 14:03

tomasr8 reviewed Apr 2, 2024

View reviewed changes

indico/modules/rb/models/reservation_occurrences.py Show resolved Hide resolved

moliholy force-pushed the booking-occurrence-linking branch from 98eba97 to b9ea67b Compare April 5, 2024 07:12

ThiefMaster reviewed May 2, 2024

View reviewed changes

indico/modules/rb/controllers/backend/bookings.py Show resolved Hide resolved

indico/modules/rb/client/js/modules/calendar/CalendarListView.jsx Outdated Show resolved Hide resolved

José Molina and others added 20 commits May 3, 2024 14:35

Link booking occurrences to events

5408f5f

Improve booking and event access logic

3033b3b

Improve parameter handling

3b272f5

Improve readability

f62765c

improve sql speed

4e36501

include boundaries

09552e3

fix redux error

8644b48

add tests for can_link

ea131a7

improve occurrence linking GUI

ddef9d1

add modal to confirm linking

316963b

Minor fixes

1b9c0a9

Do not leak link info

18311a6

improve sqlalchemy autoschema

100eaea

Use better RH name

8875d53

Log linking to event log

7c75682

Improve error messages

e9c5420

Also use a tuple instead of a list in overlaps check

WIP: Take admin override mode into account

9bc6e16

Fix linking confirm modal using wrong occurrence

5e6edba

Fully take admin override mode into account

d67336a

Do not wrap whole method body in if block

0bcbe75

ThiefMaster force-pushed the booking-occurrence-linking branch from 455abc8 to 0bcbe75 Compare May 3, 2024 12:35

ThiefMaster added this to the v3.3 milestone May 3, 2024

ThiefMaster added 2 commits May 3, 2024 14:38

Add changelog entry

c97c897

Move non-access-related checks into process_args

958d266

`check_access` is meant for access checks, not for general validation

ThiefMaster approved these changes May 3, 2024

View reviewed changes

ThiefMaster merged commit aac9e4f into indico:master May 3, 2024

ThiefMaster reviewed May 31, 2024

View reviewed changes

Link booking occurrences to events #6243

Link booking occurrences to events #6243

Uh oh!

Conversation

moliholy commented Mar 20, 2024

Screenshots

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moliholy commented Mar 26, 2024

Uh oh!

ThiefMaster commented Mar 26, 2024

Uh oh!

tomasr8 commented Mar 26, 2024

Uh oh!

Uh oh!

moliholy commented Mar 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moliholy Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomasr8 commented Apr 2, 2024

Uh oh!

Uh oh!

moliholy commented Apr 5, 2024

Uh oh!

ThiefMaster commented May 2, 2024

Uh oh!

Uh oh!

Uh oh!

OmeGak commented May 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moliholy May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

moliholy Apr 5, 2024 •

edited

Loading

moliholy May 31, 2024 •

edited

Loading