ice-cube-ruby / ice_cube

Ruby Date Recurrence Library - Allows easy creation of recurrence rules and fast querying
MIT License
2.41k stars 358 forks source link

Schedule#exception_rules behavior is incorrect #427

Closed dsdshcym closed 7 years ago

dsdshcym commented 7 years ago

it should be:

    # Get the exception rules
    def exception_rules
      @all_exception_rules.select { |r| r.is_a?(SingleOccurrenceRule) }
    end

Though it would be better if there is an ExceptionRule class or something alike.

avit commented 7 years ago

"Exception Rules" and "Exception Times" are different.

Both are handled internally as types of "Rules", but different types go into each category:

Reading exception_rules and exception_times should only give the different types back, so I don't think this is a bug: there are different methods for accessing these.

Exception Rules are deprecated in the newer iCalendar RFC 5545, (see the older iCalendar RFC 2445 for reference). For now we continue to support them.

We can discuss what you're looking for, if there's an underlying reason for this issue, but let's close the PR #428 for now.

dsdshcym commented 7 years ago

Thanks a lot for your explanations!!

I'm not familiar with iCal specifications, thanks for the references, I'll check it out.

I'm using ice-cube for scheduling recurrent event in a project I'm working on.

The original issue that got me into this was like this:

  1. Create a schedule with recurrent rule (say daily)
  2. Add a exception time
  3. Update =schedule.start_time=
  4. The exception rule is no longer effective now (Because it's a SingleOccurrenceRule for a specific time and =schedule.start_time= was changed)

The script to reproduce this:

    schedule = IceCube::Schedule.new(Time.parse('12:00'))
    schedule.add_recurrence_rule(IceCube::Rule.daily)

    schedule.next_occurrence
    # => 2017-10-17 12:00:00 +0800

    schedule.add_exception_time(schedule.next_occurrence)
    # => 2017-10-17 12:00:00 +0800
    schedule.next_occurrence
    # => 2017-10-18 12:00:00 +0800

    schedule.start_time = Time.parse('13:00')
    # => 2017-10-17 13:00:00 +0800
    schedule.next_occurrence
    # => 2017-10-17 13:00:00 +0800
    # But I want the exception_rule to be synced with schedule.start_time
    # So the next_occurrence will be 2017-10-18 13:00:00 +0800

The solution I came up with was to get all exception time and update their times one by one, which is not very efficient but we can take it for now.

So for this solution, I tried to use @exception_rules@ to get all the exception rules but it didn't work (since I was using @add_exception_time@ to add rule)

(Maybe you can suggest some better solutions for my requirement?)

Then I submitted this issue and the PR.

I think the problem here is about naming, I thought:

  1. @exception_rules@ was about getting all exception rules instead of all except single times
  2. @exception_times@ was using @exception_rules@ to get all rules and convert them into times

And I double checked SingleOccurrenceRule, it inherits Rule but it doesn't implement to_ical method, which is a bit odd to me.

Maybe we can come up with better names to distinguish these methods and classes better? (Or is there any background I'm missing here?)

Andrew Vit writes:

"Exception Rules" and "Exception Times" are different.

Both are handled internally as types of "Rules", but different types go into each category:

  • add_recurrence_rule: add all except single times (Daily, etc.) to @all_recurrence_rules
  • add_recurrence_time: add single times only to @all_recurrence_rules
  • add_exception_rule: add all except single times (Daily, etc.) to @all_exception_rules
  • add_exception_time: add single times only to @all_exception_rules

Reading exception_rules and exception_times should only give the different types back, so I don't think this is a bug: there are different methods for accessing these.

Exception Rules are deprecated in the newer iCalendar RFC 5545, (see the older iCalendar RFC 2445 for reference). For now we continue to support them.

We can discuss what you're looking for, if there's an underlying reason for this issue, but let's close the PR #428 for now.

avit commented 7 years ago

Aha, I see... everything is a static time, so you could write your own change_schedule_time(new_start_time) method, and loop over the exception_times & recurrence_times to add the difference.

(These are just normal Time objects outside of the schedule, no need to worry about SingleOccurrenceRule.)

It might be possible for us to have a schedule.change(days: 7, hours: -8) for example, which would move everything... but I'm not sure how other rules that are set up (e.g. day_of_month) should interact with this so maybe it's better left up to you.

dsdshcym commented 7 years ago

Yes, I've implemented a similar solution like what you said.

I think a better solution could be excluding occurrences in order (like excluding first occurrence, second occurrence, etc.) instead of excluding a specific time.

But I don't have any background knowledge of iCal specs. This solution might not be iCal compatible.