Roomify / bat_drupal

Booking and Availability Management Tools for Drupal 7 and Drupal 8
33 stars 7 forks source link

Editing event subtracts/adds 1 minute to end date #35

Closed derimagia closed 8 years ago

derimagia commented 8 years ago

This is obviously intended but it's really confusing to me. Why do you add/subtract 1 minute to the end date when editing an event?

Subtracting: https://github.com/Roomify/bat_drupal/blob/69df2b85d7acdb0c4414015fe5c28b7ac7a1c395/modules/bat_event/bat_event.admin.inc#L433-L436

Adding: https://github.com/Roomify/bat_drupal/blob/69df2b85d7acdb0c4414015fe5c28b7ac7a1c395/modules/bat_event/bat_event.admin.inc#L199-L200

It says because BAT needs it to not be included. Seems very weird, however if it truly is needed then it should probably be removed to the entity load and entity save methods in the controller...

Right now I'm having issues because I"m doing queries on the database using EFQ and it's off by the 1 minute.

acrollet commented 8 years ago

@derimagia I agree it's quite confusing, if you feel like taking a look at how event dates are actually stored in the DB by bat it will probably make a bit more sense. Basically, we have one minute granularity, and the DB structure precludes having two events occupying the same time. (Which would be the case if, for example, you had one event ending at 16/02/2016 12:00 and another starting at 16/02/2016 12:00)

We'll certainly consider placing the addition/subtraction in the controller - however I don't think that would solve your issue with performing EFQs, yes?

Can I ask what exactly you're trying to do with the EFQs? This would seem like a good place to leverage the BAT API to return events meeting whatever specification. If you've got a use case that the API doesn't meet, we're certainly interested in hearing about it.

thanks, Adrian

cc @istos

derimagia commented 8 years ago

For a little bit of up-front knowledge, take a look at Issue 21

Essentially what I need is a site where a Specialist can schedule "Availability" (So say, tomorrow at 5am - 11pm). Then a regular member can schedule appointments in this "Availability".

I need REST / Service APIs for alll of this because it has a mobile app too.

I need a bunch of custom endpoints to schedule "availability", - CRUDing them as well as making sure they don't overlap with other availability.

I'll then need an endpoint to check for time slots available. Specialists have a "length of appointment" set so it queries for Availability, cross checking booked appointments and spits out all the time slots available: With 15min gaps and 1 hour length of apt but you have a apt at "1:30PM" you'd get back "11AM, 12:15PM, 2:45PM". Kind of Open Table style.

There will eventually need to be a web interface to display avail. and appointments on a calendar. I'll then need endpoints to actually book places, cancel appointments, modify field on appointments, etc.

Right now It's set up by having two "event types" - availability and appointment and every Specialist user has a unit attached to it automatically (from code). Most of the stuff I've done so far has been done custom.

Thoughts on how I can use BAT? At this point I kind of feeling like I'm working next to / against BAT instead of with... but I think that may just be because I don't know a lot of it.

istos commented 8 years ago

Just a comment on why we add/remove a minute before we show the event edit form. People are used to seeing events as 1230-1300 and then 1300-1330 and don't think twice about the very serious overlap there :-) As BAT handles minute granularity for us the events are actualy 1230-1259 (with the last minute included) and 1300-1329.

Now, users would be confused if they saw this in their edit forms. That is why we always add or remove a minute based on what we are about to show users. I prefer the entity controller to handle things as BAT expects (and not deal with adding/removing a minute if it doesn't have to) since that can be used by the API as well. Hope this makes sense!

acrollet commented 8 years ago

@derimagia it definitely feels like you're working against BAT - we've substantially improved the documentation, I think a look at http://docs.roomify.us/bat/index.html would be well worth your time. (Particularly http://docs.roomify.us/bat/drupal/take_bookings.html)

The endpoints provided by bat_api should already get you a substantial portion of the way, since you can create and edit events and units. We'd definitely be open to any patches enhancing CRUD capabilities there.

You should only need a single event type (Availability) for your use case, with fixed states for unavailable, available and appointment. Depending whether the appointment start times are fixed, you could either programatically create events with the appointment state when available times are set, or create BAT constraints to control how slots are made available.

example BAT constraint: https://github.com/Roomify/bat/blob/master/src/Constraint/MinMaxDaysConstraint.php

derimagia commented 8 years ago

Thanks for the responses. I read through the documentation that I missed before. I didn't see anything in there about constraints, is there something written or should I just take a look at code?

I'll take a look if I can switch to the Search API for searching availability. I can see that being an improvement for speed since I was worried about checking available time slots on demand. I may not be able to cache them in a search db though because the timeslots may be dependent on the distance between appointments (address/location). For now I'll keep this in the back of my head since it doesn't look like there is a good way for this dynamic aspect of it.

I do a lot of stuff with services so I'll take a look at the API as well. I already saw it but we need to do some custom work in the service so I may not be able to use the service outright.

I also will remove the "appointment" event and probably just make an eck entity or custom entity for a "booking", which is very similar to what I'm doing now. I still need to research/do the entire "commerce" part of it, so that is useful.

@acrollet in your example, what would the fixed state "unavailable" be used for. Wouldn't the absence of an event be used for availability or is it used to override a day of availability?

@istos I'll keep the 1 minute thing in mind. Appreciate the explanation. I probably will still need to using custom queries because of the application having some weird logic, but I'll try to avoid them as much as possible.

One point though:

I prefer the entity controller to handle things as BAT (...)

Right now the Entity Controller doesn't handle it, the forms do and bat_api does. If I set the date to "2016-03-12 22:00:00" in the event and then print out bat_event_load($event_id)->end_date I get "2016-03-12 21:59:00". Isn't this the opposite of what you would prefer? It's a bit weird.

acrollet commented 8 years ago

in your example, what would the fixed state "unavailable" be used for. Wouldn't the absence of an event be used for availability or is it used to override a day of availability?

BAT states are what you make of them :) In my example, the 'unavailable' state would mostly be a convenience as a way to visually represent times that are not available for appointments. If you liked, you could make it a blocking state and then other events could not be created during unavailable times without first modifying the event in the unavailable state.

I prefer the entity controller to handle things as BAT expects (and not deal with adding/removing a minute if it doesn't have to) since that can be used by the API as well.

I believe what @istos is saying here is that he prefers event times to be handled in the way that BAT (the library, rather than the drupal module) expects to receive them. Given that you can also interact directly with the BAT library, it makes sense for times to remain consistent whether your code is dealing with Drupal entities or directly with BAT units/events.

shameless plug: we're available for consulting services if you find you don't have the time to make the customizations you wish.

cheers, Adrian

derimagia commented 8 years ago

Thought so on the states, thanks for all the clarification and info. Will keep you guys in mind. Still curious on @istos response, because it seems like the fact that the +/- 1 minute not being in the Entity Controller is still a bug that should be fixed.

I'll be fine with looking to add a pull request this by moving it when I get a chance.

acrollet commented 8 years ago

We're not likely to accept a PR moving the +/- one minute code given the above, so I wouldn't spend time writing it. Here's a counterpoint that will hopefully help cast it in a new light, with the presupposition that we have made this change to the controller:

Suppose that I am a developer who is familiar with the BAT php library and how it deals with dates. I start a new application dealing with booking short periods of time in Drupal, but write some custom code that interacts directly with Drupal events. I then find that a minute is being subtracted from my end dates, and file a new bug ticket in the bat_drupal issue queue with a PR to stop this odd and unexpected behavior. :)

derimagia commented 8 years ago

Are you sure you understand what i mean by moving it to the Entity Controller? Right now it's completely unexpected that when I add an event in the UI and then I entity_load the event it doesn't have the correct date on it.

Not seeing the downside as it's the exact thing that happens right now but instead of doing it manually on form logic and in the bat_api module it's doing it the switch on ALL save/load logic automatically in drupal.

acrollet commented 8 years ago

I don't want to turn this into a back and forth, but I do want to make clear that I understand what you mean. The point is that showing different times in the UI is a convenience for users.

I can just as easily say that it would be completely unexpected to create a new BAT event in code, save it, and then find that the event end time saved in the database didn't match what I specified when I created the event. It comes down to the fact that it's a design decision. Given the mismatch between user expectations and the way we have to store dates in a non-overlapping manner, we had to choose to between adjusting user expectations (hard) or making an adjustment somewhere. We made the decision that the adjustment should be made at the display layer. This also gives other applications built on BAT for Drupal the freedom to make a different decision, and means that all code written against BAT remains consistent in behavior; whether it's calling the Drupal layer API or the BAT API directly.

derimagia commented 8 years ago

Since this doesn't seem likely to happen, can you please share with me a link to some type of documentation to why this one minute thing is necessary?

istos commented 8 years ago

Hi :-) what exactly are you finding so hard with regards to "this one minute thing". BAT considers events to include the last minute. For example: 12:30 - 13:29 - Event 1 13:30 - 14:29 - Event 2

Say the two events above represent room bookings. The way a user thinks of these events is:

12:30 - 13:30 - Event 1 13:30 - 14:30 - Event 2

So we add a minute when we show the event to the user to meet their expectation. However, we cannot store it like that because then they would be overlapping events (they both have 13:30) in them.

Does that make sense?

derimagia commented 8 years ago

I understand why you are adding/subtracting one minute on Drupal's side. I'm making/updating bat events myself in code so I'll need to do the same thing, so I would like to understand why BAT considers the last minute included

istos commented 8 years ago

As acrollet mentioned ultimately it is a design decision. Our granularity is 1 minute down to the way we store data. So if we say a certain minute has a certain state that means we need to include it.

It makes sense for me to see events as:

12:30 - 13:29 - Event 1 13:30 - 14:29 - Event 2

and know that that means up to and including 13:29 or 14:29. We could have chosen to go with a different approach but that didn't seem as coherent at the level of BAT especially when you think of even smaller events and events that do not finish on clean :30 or :00 times. e.g:

12:31 - 12:42 12:43 - 13:13

etc.

However, it is one of those things that we had to pick something that made sense to us. And we did. Hopefully it is not causing too many headaches!

derimagia commented 8 years ago

Thanks for the info. I haven't ran into issues yet but I need to create these events from my own custom services and want to make sure everything is correct. Thanks for your input