-
Notifications
You must be signed in to change notification settings - Fork 416
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
Booking form extra fields #6126
Booking form extra fields #6126
Conversation
indico/modules/rb/client/js/common/bookings/BookingEditForm.jsx
Outdated
Show resolved
Hide resolved
indico/modules/rb/client/js/common/bookings/BookingEditForm.jsx
Outdated
Show resolved
Hide resolved
677e775
to
b5fc63e
Compare
edfa59a
to
fd9d5df
Compare
4a47a85
to
7a2e0c1
Compare
b104e54
to
d191035
Compare
@@ -53,14 +53,19 @@ def past_booking_occurrences_are_cancelled(dummy_user, create_reservation): | |||
start_dt = datetime.today().replace(hour=8, minutes=30) - timedelta(days=2) | |||
end_dt = datetime.today().replace(hour=17, minutes=30) + timedelta(days=4) | |||
reservation = create_reservation(start_dt=start_dt, end_dt=end_dt, repeat_frequency=RepeatFrequency.DAY) | |||
new_reservation = split_booking(reservation, { |
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.
Oh fun, this unit test is completely broken and has never been executed (opened #6210 for it since I don't feel like touching that...). Anyway your change in here looks fine.
@VojtechPetru are you able to share either an example plugin that uses these signals or your real plugin (even if privately e.g. via email or by granting me access to a private github repo)? I don't see any likely breakage, but I'd prefer to actually test it before merging, and I don't think it's a good use of my own time if I have to write my own test plugin for this while you may already have something! :) |
Sure! I've sent you an invite to the repo. Thank you :) |
2c297ef
to
cbb82d7
Compare
When testing with your plugin, I noticed some minor issues:
For (2) the cleanest solution would be to integrate the fields with final-form: This would require you to use Here's a patch that could provide a good starting point. You most likely also need to find a good way to get the diff --git a/indico/modules/rb/client/js/common/bookings/BookingEdit.jsx b/indico/modules/rb/client/js/common/bookings/BookingEdit.jsx
index b7df8d365a..28e38cdcaf 100644
--- a/indico/modules/rb/client/js/common/bookings/BookingEdit.jsx
+++ b/indico/modules/rb/client/js/common/bookings/BookingEdit.jsx
@@ -77,7 +77,6 @@ class BookingEdit extends React.Component {
willBookingSplit: false,
calendars: null,
timelineError: null,
- extraFields: null,
};
}
@@ -180,7 +179,7 @@ class BookingEdit extends React.Component {
};
renderBookingEditModal = fprops => {
- const {submitting, submitSucceeded, hasValidationErrors, pristine} = fprops;
+ const {form, submitting, submitSucceeded, hasValidationErrors, pristine} = fprops;
const {booking, onClose, actionButtons, isOngoingBooking} = this.props;
const {room} = booking;
const {
@@ -314,6 +313,7 @@ class BookingEdit extends React.Component {
user,
reason,
recurrence,
+ extraFields,
internalNote,
} = data;
const {
@@ -326,7 +326,6 @@ class BookingEdit extends React.Component {
isAdminOverrideEnabled,
} = this.props;
const [repeatFrequency, repeatInterval] = serializeRecurrenceInfo(recurrence);
- const {extraFields} = this.state;
const params = {
start_dt: `${startDate} ${startTime}`,
end_dt: `${endDate ? endDate : startDate} ${endTime}`,
diff --git a/indico/modules/rb/client/js/common/bookings/BookingEditForm.jsx b/indico/modules/rb/client/js/common/bookings/BookingEditForm.jsx
index 7be09936ad..efa0b207e3 100644
--- a/indico/modules/rb/client/js/common/bookings/BookingEditForm.jsx
+++ b/indico/modules/rb/client/js/common/bookings/BookingEditForm.jsx
@@ -388,6 +388,11 @@ class BookingEditForm extends React.Component {
required={requireReason}
disabled={submitSucceeded}
/>
+ <FinalTextArea
+ name="extraFields.test"
+ nullIfEmpty
+ placeholder={Translate.string('Test')}
+ />
</Segment>
{renderPluginComponents('rb-booking-form-extra-fields', {
room, |
Hello @ThiefMaster 👋 The suggested |
Considering that this is just a plugin API where there's a good chance nobody else will use it anytime soon, I'd be OK with merging this as-is for now. I'll see if I can find a way so changes to custom fields can at least mark the form as dirty so the submit button becomes (permanently) enabled once something is changed there... |
{renderPluginComponents('rb-booking-form-extra-fields', { | ||
room, | ||
booking, | ||
formProps, |
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.
I'm replacing this (which you do not use anyway in your plugin) with disabled
(submitting || submitSucceeded
) here and adding this in booking creation. You should use that in your plugin to disable the custom fields.
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.
ok, will do! 👍
cbb82d7
to
1c33141
Compare
While testing I noticed another problem: when no core fields are changed, the Reservation.modify method exits early so the signal never gets called. So I'm starting to wonder whether it wouldn't make more sense to revert the changes made to pass the request data to the booking-related signals, and instead rely on the schema-level signals (pre_load etc)? |
Thanks for such an elaborate review @ThiefMaster! 👑 I'll check it on Monday, but from a brief look it seems like something which could work. Just to be sure, we're talking about this part of code, right? What worries me a little bit is coupling the plugin logic so tightly to the validation schema. |
something like this could also work: if changes:
# the usual logging here
elif 'extra_data' not in data:
return that way we'd avoid creating empty log entries (if nothing changes that the core is aware of), but still keep going if any extra data is present... |
but i'm wondering why you changes work at all :D the default when using a marshmallow schema for processing request data is to ignore any unknown fields, and you did not add |
1c33141
to
febb6e5
Compare
You're right 👍 Giving it a second thought, what I like about the interceptable As for the schema and why the plugin works, it should be these few lines which extend the validation schema when the plugin is initialized |
Ah I see, you patch the core schema from your plugin, that's why it works. CreateBookingSchema._declared_fields['extra_fields'] = mm.fields.Nested(
schemas.ReservationExtraFieldsSchema,
required=False,
allow_none=True,
) It also means that the core code as it is right now cannot look at BUT: The core JS code is already aware of When making that change it may also be a good idea to not pass the full |
Hello @ThiefMaster 👋 |
Could you rebase it and fix the conflicts? |
3164077
to
c9c134c
Compare
27a6c80
to
31aba5f
Compare
if (has_slot_changed and not room.can_book(session.user, allow_admin=False) and | ||
room.can_prebook(session.user, allow_admin=False) and self.booking.is_accepted): | ||
self.booking.reset_approval(session.user) | ||
else: | ||
new_booking = split_booking(self.booking, new_booking_data) | ||
new_booking = split_booking(self.booking, new_booking_data, extra_fields=args.get('extra_fields', {})) |
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.
Is the .get()
really needed here? I'd expect it to be missing from the request data (in that case the schema uses the load_default
to give you {}
) or set to a value (which is always a dict anyway).
…ceptables; renames
Removed passing the full formProps since those should not be necessary.
36f802c
to
dde1050
Compare
Hello 👋
Example:
![image](https://private-user-images.githubusercontent.com/54205005/296197743-cf625444-1bff-4121-8088-f4c4d1b069fc.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQwOTQ5NDMsIm5iZiI6MTcyNDA5NDY0MywicGF0aCI6Ii81NDIwNTAwNS8yOTYxOTc3NDMtY2Y2MjU0NDQtMWJmZi00MTIxLTgwODgtZjRjNGQxYjA2OWZjLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MTklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODE5VDE5MTA0M1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTljYjExYjI0MzYwODI3MzMyODdjZTkxMGVmZjcwZjc4NDc4ODI5NzU1MzdkYzJmYmQ1MTdkNzY3NzVjZDkzNDkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.I4gMHxDTHlyNOibh2Aoe0dn0nWl0lmkb0GGEkK0ibXU)