Roomify / bat_drupal

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

Dealing with Events overwriting each other #42

Closed derimagia closed 7 years ago

derimagia commented 8 years ago

I have an issue and just wanted to see if it is a known thing in Bat.

If you make an event from 1PM - 2PM with a state: "first_state"

Then you make another event from 12:30PM to 2:30PM with state "second_state" the first state will be gone when you try to query it using getEventsItemized.

Not only that but if you were to delete the second state, the first one won't have a record of it. I understand an event not having two events at once when itemizing, but shouldn't the first state come back when you deleted the one that overwrote it?

acrollet commented 8 years ago

@derimagia - I'm guessing that you're directly creating events with the API, vs. the bat_drupal UI?

As far as https://github.com/roomify/bat itself is concerned, state for a given time is only stored based on the most recent event entered - this allows simplification of the API and helps performance. It's assumed that an application built on bat will take this into account.

If you want to have overlapping events with different states, that's a use case we didn't take into account - we assume that a single unit will be in a single state at any given time. Can you expand on your use case?

cc @istos in case I missed something.

derimagia commented 8 years ago

I figured that was the approach - I'm creating drupal entity bat events programmatically , not through the UI. On entity save batStore is called but it doesn't resave things that could have been under it after it's deleted.

However I think you could easily duplicate it in the UI by adding an event with one state, then another one in the middle with a different state, and then deleting the first one.

Specific use case: https://github.com/Roomify/bat_drupal/issues/35#issuecomment-185357799

Essentially have events with 3 states on an event - available, booked, and traveling. Booked and traveling are both blocking states.

acrollet commented 8 years ago

This is a good point, it's possible to create an event via the BAT UI that overrides a blocking state, we shouldn't allow this. I've added a ticket in our tracker.

As for restoring the state of a previous event, once the new code is in place it shouldn't be possible to have overlapping events unless both states are non-blocking. In that case, I'm not sure it's obvious that the state should revert to a previous event's state if a later event is deleted - reverting to the default state seems like as good a choice as any to me.

acrollet commented 8 years ago

See https://github.com/Roomify/bat_drupal/commit/41edcb7fb9113caaa7b11512650268a5ecd5b173 and https://github.com/Roomify/bat_drupal/commit/ea392b79c8c7f740632595b3849e195c609d2c4c

We talked over the issue where the calendar won't retain the state of non-blocking events when another overlapping event is deleted. When we get a chance we will be carefully documenting these things but in the meantime the short answer is that if this matters for your use case, you should mediate the experience of users editing events.

Hope this helps.

derimagia commented 8 years ago

Very much a shame that you don't want to make it more flexible and instead place weird limitations on how you enter data.

For people looking at this in the future, I easily implemented the fix on my own repo: https://github.com/derimagia/bat_drupal/commit/2f80bdc5f11a9fe861ffb4decc0161310c9f7feb

acrollet commented 8 years ago

@derimagia we're certainly open to making it flexible, however what you seem to want is to change the existing behavior rather than add flexibility. If you'd like to submit your code as a PR and add an option to the BAT configuration settings to allow users to choose their behavior, I think we'd be highly likely to accept it.

derimagia commented 8 years ago

When I have time I'll finish this up.

It may be that I don't understand fully, I don't see the point in not recovering the event state, can you give me a scenario where you want this?

derimagia commented 8 years ago

Let me expand this a little bit, just want to give some thoughts after using it for a while now.

The reason I needed it is because I'm dealing with availability which needs to exist under events that can be cancelled. But, this aside - I kind of feel like BAT caches and dissects events into a calendar type of SQL table. But, one issue is that if you don't recover the events, you can't delete the BAT tables from the drupal entities since you can't be certain if an entity was created and then deleted which would throw everything off.

I didn't notice any other areas that would behave like this, maybe there are others. I feel like the BAT tables are very fragile. After needing to develop in it's core in Drupal i've corrupted it a few times, and there wasn't an easy way to re-create it.

That aside I still think it's very useful and you guys did an awesome job on it, I just want to make sure it makes sense in a Drupal context so that it's flexible for both site builders and site developers.

acrollet commented 8 years ago

@derimagia I think it's great for users to have a choice because ultimately we want BAT to be as flexible as possible - here's an example of a use case where reverting to the default is best:

A unit represents a hotel room, it's default state is 'Available'.

An event is created for certain dates with a state of 'Unavailable'. Another event with the same end date is later created with a state of 'Remodeling'. The remodeling process finished early (ok, that part is unrealistic ;)) and the owner drags the end of the event to shorten it so that it now ends today.

The desired state from today on would now be 'Available' for most hotel owners.

derimagia commented 8 years ago

Not sure I get why you need to de-sync drupal and BAT for that though, depending on the UI, wouldn't you just either merge them and not have them overlapping or when you drag to shorten the event it would also shorten the unavailable event?

But what you don't want to happen is shorten the renovating and have an unavailable event under it, however the calendar won't show it because it's only in drupal and not in the BAT databases. But if that's your guy's design decision then alright, thanks for the info

derimagia commented 8 years ago

Can I get a response to this? Would be nice to be able to merge my various improvements back in to contribute, but I'd like to make sure I understand everything