Skip to content

Support recurrence for TimeWindowFilter - #266

Merged
Zhiyuan Liang (zhiyuanliang-ms) merged 63 commits into
mainfrom
zhiyuanliang/recurring-time-window
Apr 23, 2024
Merged

Support recurrence for TimeWindowFilter#266
Zhiyuan Liang (zhiyuanliang-ms) merged 63 commits into
mainfrom
zhiyuanliang/recurring-time-window

Conversation

@zhiyuanliang-ms

@zhiyuanliang-ms Zhiyuan Liang (zhiyuanliang-ms) commented Sep 22, 2023

Copy link
Copy Markdown
Member

Why this PR?

#194

Visible changes

Add-on "Recurrence" parameter for TimeWindowFilter using Outlook-style schema:
https://learn.microsoft.com/en-us/graph/outlook-schedule-recurring-events#using-patterns-and-ranges-to-create-recurring-events

The modification we made to the original outlook schema:

  1. We remove the "StartDate" property in the RecurrenceRange and we force the "Start" property of the time window to be a valid first occurrence (there are validations for "Start" property, checking whether it follows the recurrence pattern).
  2. We remove the "RecurrenceTimeZone" property in the RecurrenceRange and we will use the UTC offset in the "Start" property.
  3. We will only support "Daily" and "Weekly" recurrence pattern type: Support recurrence for TimeWindowFilter #266 (comment)

Some discussion can be found in #256

Other comments

The main logic of how to check whether current time is within any recurring time window:

  1. Find the previous occurrence of the recurring time window, let's call it prevOccurrenceStart
  2. Check whether the current time: time is within the time window: prevOccurrenceStart ~ prevOccurrenceStart + End - Start


const int WeeklyUnitIntervalDuration = 7; // in days
const int MonthlyUnitIntervalDuration = 28; // in days
const int YearlyUnitIntervalDuration = 365; // in days

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should make all these const strings public. You have the RecurrencePattern public, but not their values.

Comment thread src/Microsoft.FeatureManagement/FeatureFilters/TimeWindowFilter.cs Outdated
@zhiyuanliang-ms Zhiyuan Liang (zhiyuanliang-ms) changed the title In Progress: Outlook-style schema for recurrence Outlook-style schema for recurrence Sep 29, 2023
@zhiyuanliang-ms
Zhiyuan Liang (zhiyuanliang-ms) force-pushed the zhiyuanliang/recurring-time-window branch 2 times, most recently from 2cb3e59 to f8d0c34 Compare September 29, 2023 11:50
Comment thread src/Microsoft.FeatureManagement/FeatureFilters/TimeWindowFilter.cs Outdated
Comment thread src/Microsoft.FeatureManagement/FeatureFilters/Recurrence/RecurrencePattern.cs Outdated
@zhiyuanliang-ms
Zhiyuan Liang (zhiyuanliang-ms) changed the base branch from preview to main November 14, 2023 07:24
Comment thread src/Microsoft.FeatureManagement/FeatureFilters/Recurrence/RecurrenceEvaluator.cs Outdated
Comment thread src/Microsoft.FeatureManagement/FeatureFilters/Recurrence/RecurrenceEvaluator.cs Outdated
Comment thread src/Microsoft.FeatureManagement/FeatureFilters/Recurrence/RecurrenceEvaluator.cs Outdated
Comment thread src/Microsoft.FeatureManagement/FeatureFilters/Recurrence/RecurrenceEvaluator.cs Outdated
Comment thread tests/Tests.FeatureManagement/RecurrenceEvaluator.cs Outdated
Comment thread src/Microsoft.FeatureManagement/FeatureFilters/TimeWindowFilter.cs Outdated
Comment thread src/Microsoft.FeatureManagement/FeatureFilters/TimeWindowFilter.cs Outdated
Comment thread src/Microsoft.FeatureManagement/FeatureFilters/TimeWindowFilter.cs Outdated
Comment thread src/Microsoft.FeatureManagement/FeatureFilters/TimeWindowFilter.cs Outdated
@zhiyuanliang-ms
Zhiyuan Liang (zhiyuanliang-ms) force-pushed the zhiyuanliang/recurring-time-window branch 2 times, most recently from 66b9ca7 to ac8f8f4 Compare April 16, 2024 16:50
Comment thread src/Microsoft.FeatureManagement/FeatureManagementBuilder.cs Outdated
}

[Fact]
public async void RecurrenceEvaluationThroughCacheTest()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we check if there's an entry in cache?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just sent a PR #445 to test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants