Support recurring time window filter for TimeWindowFilter

[zhiyuanliang-ms]

Implemented the CrontabField and CrontabExpression classes and wrote some unit tests

[juniwang]

are we using one single regex to test all fields? this makes the regex very complex and hard to understand. Any way to make it more comprehensible?

[juniwang]

Also the pattern should be constant. Or a static Regex for reuse.

[zhiyuanliang-ms]

I can add more comments to explain this regex. But I think using one single regex can make the code tidy and simple.

[jimmyca15]

Please avoid regex. Regex has been the cause of countless server hangs. As such, by principle I always seek to avoid it. In this case, I almost expect it to be easier to pattern match without regex.

[zhiyuanliang-ms]

Re-implemented the CrontabField.TryParse() without using regex.

[amerjusupovic]

Could you explain the 31-1/100 part of the day value? Does cron support the day of the month wrapping around from 31 to 1?

[zhiyuanliang-ms]

The IBM doc and man page of crontab(5) both don't mention whether this is valid.
https://crontab.guru/ this website will consider this invalid. (By the way, this website recognized the step value 100, it only denies range like 31-1)
However, the Quartz and NCronTab libs will treat "31-1/100" as a valid expression.
I think the range 31-1 and step value 100 can be interpreted as the from 31st to 1st day of month per 100 days, which will give us only the 31st day of the month. From the code perspective, this makes sense.
This is not a critical error, so I believe it should be toleranted by the crontab parser.
I can add some extra logics to check whether the step value is within the max value of crontab field and check whether the range is from a smaller number to a a larger number.
What do you think? Should we treat "31-1/100" as an invalid one?

[amerjusupovic]

I didn't realize NCronTab treated that expression as valid, so it makes more sense to stick with what the NCronTab implementation is and treat that expression as just the 31st day. As long as it's clear that we're sticking with this library then I think it's ok. Thanks for the explanation!

[zhiyuanliang-ms]

I think "first-last/step" should be interpreted to something like a for loop: for (int i = first; i<= last; i+=step). If we consider it in this way, everything will make sense.

[zhiyuanliang-ms]

Update: The range like "31-1" should be forbidden. The reasons are as follow:

1. The portal should validate the Crontab expression typed by user. If such reversed range is allowed, user may be confused because they may not consider the range in the for-loop manner.
2. Although Quartz and NCronTab will treat this as a valid expression, I found that it might be a bug or a corner case they didn't handle. After more tests, I found that the behavior of NextOccurence(), IsSatisfiedBy(DateTimeOffset) would be extremly strange when both libraries were handling the Crontab like "* * 31-1/100 * *".

For example:
UTs for NCronTab:

[InlineData("* * 31-29/2 * *", "Tue, 29 Aug 2023 00:00:00 +00:00", true)]
[InlineData("* * 31-1/30 * *", "Thu, 31 Aug 2023 01:00:00 +00:00", true)]
[InlineData("* * 31-1/30 * *", "Fri, 1 Sep 2023 01:00:00 +00:00", true)]
[InlineData("* * 31-1/31 * *", "Fri, 1 Sep 2023 01:00:00 +00:00", true)]
[InlineData("* * 31-1/31 * *", "Thu, 31 Aug 2023 01:00:00 +00:00", false)]
[InlineData("* * 31-1/100 * *", "Thu, 31 Aug 2023 01:00:00 +00:00", false)]
[InlineData("* * 1-31/100 * *", "Fri, 1 Sep 2023 01:00:00 +00:00", true)]
[InlineData("* * 1-31/100 * *", "Thu, 31 Aug 2023 01:00:00 +00:00", false)]
public void IsCronSatisfiedByTimestamp(string cron, string timeString, bool expected)
{
    Assert.Equal(expected, IsSatisfiedBy(cron, timeString));
}
private static bool IsSatisfiedBy(string cron, string timeString)
{
    CrontabSchedule schedule = CrontabSchedule.Parse(cron, new CrontabSchedule.ParseOptions { IncludingSeconds = false });

    DateTime dateTime = DateTime.Parse(timeString);
    DateTime withoutSeconds = new DateTime(dateTime.Year, dateTime.Month, dateTime.Day, dateTime.Hour, dateTime.Minute, 0);
    DateTime timeAfter = schedule.GetNextOccurrence(withoutSeconds.AddMinutes(-1));
    if (timeAfter.Equals(withoutSeconds))
    {
        return true;
    } 
    else
    {
        return false;
    } 
}

UTs for Quartz:

[InlineData("* * * 31-1/100 * ?", "Thu, 31 Aug 2023 21:00:00 +08:00", true)]
[InlineData("* * * 31-1 * ?", "Fri, 1 Sep 2023 21:00:00 +08:00", true)]
[InlineData("* * * 31-1/30 * ?", "Fri, 1 Sep 2023 21:00:00 +08:00", false)]
[InlineData("* * * 31-1 * ?", "Sat, 2 Sep 2023 21:00:00 +08:00", false)]
[InlineData("* * * 31-1/100 * ?", "Fri, 1 Sep 2023 21:00:00 +08:00", false)]
public void IsCronSatisfiedByTimestamp(string cron, string timeString, bool expected)
{
    DateTimeOffset dateTimeOffset = DateTimeOffset.Parse(timeString);
    CronExpression test = new CronExpression(cron);
    Assert.Equal(expected, test.IsSatisfiedBy(dateTimeOffset));
}

Their behaviors are super weird. I cannot understand how both libraries interpret the range "31-1/100". For me, it seems more like a bug or a corner case which is not handled.
By the way, the step value like 100 is still allowed.

[jimmyca15]

I don't see any reason to define public type for cron tab expression. I expect string to suffice.

[zhiyuanliang-ms]

I think the job for the IFilterParametersBinder.BindParameters() is to pre-process the configuration paramters to some settings that filter can use directly during the evaluation. For example, the "Start" and "End" parameters are just string in the configuration, but it will become a DataTimeOffset in the TimeWindowFilterSettings.

The FeatureManager has the _parametersCache which will cached the filter parameters and settings which are bound from the raw filter parameters. So I think the raw Crontab string should be converted into CronExpression in TimeWindowFilterSettings.

Ah, I see the reason why we should store raw Cron strings in the filter settings. In this way, we can make CronExpressionClass internal.

[jimmyca15]

I think we should use Cron Expression nomenclature that seems to be a norm.

[jimmyca15]

E.g. ["* 18-19 * * Mon"] which means the recurring time window of 18:00~20:00 on Monday

This looks more like it should belong in a doc than as a comment here. Also this seems kind of like a stray comment, was it intended to be part of the method summary?

[jimmyca15]

Ah I see, because we have comments like this on the other properties. Those seem odd. I would suggest the opposite, that we remove the stray comments from the existing properties.

[zhiyuanliang-ms]

I have removed these comments.

[jimmyca15]

I'm not expecting any new public types as part of this enhancement.

[jimmyca15]

Even better if we can use ReadOnlySpan<char> to avoid allocating new strings.

[jimmyca15]

Suggested change

	TimeSpan utcOffsetForCron = new TimeSpan(0, 0, 0); // By default, the UTC offset is UTC+00:00.
	utcOffsetForCron = settings.Start.HasValue
	? settings.Start.Value.Offset
	: settings.End.HasValue
	? settings.End.Value.Offset
	: utcOffsetForCron;
	TimeSpan offset;
	if (settings.Start.HasValue)
	{
	offset = settings.Start.Value.Offset;
	}
	else if (settings.End.HasValue)
	{
	offset = settings.End.Value.Offset
	}
	else
	{
	//
	// By default, the UTC offset is UTC+00:00.
	offset = new TimeSpan(0, 0, 0);
	}

[zhenlan]

Do we really have to support an array of crontab expressions, or is one sufficient for most scenarios? Even if someone really needs more than one, they can always add more Time Window filters with different configurations.

I'm concerned this complicates the usage of the recurring Time Window filter. I can already see this complicates the UI design.

BTW, having a 'Filters' parameter in a feature filter is also confusing.

[zhiyuanliang-ms]

The scenario when the user want to activate a feature flag during 18:00-20:00 on every weekend and during 18:00-22:00 on holidays requires a list to specify recurring time windows with different durations.
Using multiple timewindow filters may not be a good choice, since the feature flag may also use other filters(e.g. targeting), in this case, the "RequirementType" is "All".

[zhiyuanliang-ms]

I agree that the parameter name 'Filters' is kind of confusing. But the cron expression does serve as "filters" since we also have the parameter "Start" and "End" which should be considered as the timestamp when the filter starts to work("FilterStart") and when the filter ends working("FilterEnd").
The reason why we use "Filters" instead of "Recurrence" is that we will treat an empty list of "Filters" as not specified.
When "Filters" is not specifed, the TimeWindowFilter will revert to the original fixed time window filter.
The name "Filters" makes more sense.

Let's take a look at two choices:
Choice A:
"Start" : "2023-9-19-00:00",
"End": "2024-9-19-00:00",
"Filters": [ ]

Choice B:
"Start" : "2023-9-19-00:00",
"End": "2024-9-19-00:00",
"Recurrence": [ ]
If "Filters" or "Recurrence" is not specified, there is no confusion.
If "Filters" or "Recurrence" is not empty, the filter will only enable the feature during the recurring time window listed in the list "Filters" or "Recurrence". However, for the “Recurrence" choice, the parameter "Start" and "End" serve as the "RecurrenceStart" and "RecurrentEnd" instead of "FilterStart" and "FilterEnd". This will make the scenario that the "Recurrence" is specified as an empty list confusing. The function of "Start" and "End" will be inconsistent if we use name "Recurrence".
The user who uses FeatureManagement-Dotnet is expected to read the README before using it. The user who uses FeatureManagement through portal will never know the backend data schema for TimeWindowFilter unless they use "Advanced Edit"(in this case, the user should be considered as a pro who has read our doc). On the Portal, the scenarios of setting a fixed time window and setting recurring time window will have different UI and will not mention the "Filters" parameter.

For me, I will vote for the name "Filters" because it makes the role of "Start" and "End" parametes consistent. I also believe it will not cause confusion for the user who uses FeatureManagement-Dotnet lib directly.

[zhenlan]

The scenario when the user want to activate a feature flag during 18:00-20:00 on every weekend and during 18:00-22:00 on holidays requires a list to specify recurring time windows with different durations.

I'm sure we can come up with even more complex scenarios. The question is how realistic they are. Outlook doesn't support infinite recurrence patterns, just one pattern at a time. Without the array, the parameters can be simplified to "Start", "End" and "RecurrencePattern". The UI design doesn't need an add button for more than one recurrence pattern anymore.

Using multiple timewindow filters may not be a good choice, since the feature flag may also use other filters(e.g. targeting), in this case, the "RequirementType" is "All".

We know the "RequirementType" doesn't support all possible logical operations. It was a conscious decision. It's a balance between complexity and the most common scenarios. The choice of that design shouldn't be the reason for us to complicate the design of each feature filter. We have a reasonable workaround. We know there are scenarios that won't be supported out-of-box no matter how. That's where customers should consider custom filters.

[zhiyuanliang-ms]

If we do not want to use a single parameter "RecurrencePattern", then we cannot use cron expression any more. The reason is that we have to use at least three cron expressions to describe the time span 18:30`20:30. e.g. "30-59 18 * * ", " 19 * * *“ and "0-29 20 * * *".
I have proposed a lot of schema designs to describe the recurrence. Some of them are deprecated early and are not included in the design doc.
The best way to describe recurrence is to specify the duration and recurrence pattern (i.e. when we want to make the recuurence happen).
At first, I want to use two parameters "Duration" and "RecurWhen".

// 18:30-20:30 every Friday
"Duration": { 
  "Type": "Hour", 
  "Length": 2 
},  
"RecurWhen":  "30 18 * * 5"

This represents 18:00-22:00 every Friday.
This design will use cron expression in classic way where the cron expression represents the schedule (time points instead of time spans).
But Jimmy thought we can include the duration information in the cron expression, in this way the json schema will be very simple. The above configuration can be simplified to a single cron "* 18-22 * * 5".

Jimmy thought the current design achieved the balance between simplicity, offering to customers and implementation/maintenance. I completly agree with Jimmy, that's the reason why we finally decided to use the current cron design for the recurrence configuration.

Besides, the current design also provides a benefit that it will make the process to check whether the current time is within any recurring time window very simple. We only need to check whether five fields are compliant with current timestamp.
If we use the "Duration" and "RecurWhen" design, we need to compute the previous occurrence of the cron schedule which is the start of the recurring time window and then check whether the current time is within that recurring time windows.

These are some basic scenarios we want to support(you can find it in the design doc):

1. Basic Scenarios
   a) Recur during a certain time window every day

• Turn on a feature flag during 16:00~20:00 every day
  b) Recur during a certain time window on certain days of a week
• Turn on a feature flag during 16:00~20:00 on Monday and Friday every week
• Turn on a feature flag from Saturday evening to Monday morning every week
  c) Recur during a certain time window on certain days of a month
• Turn on a feature flag during 18:00~20:00 every Sunday in August
• Turn on a feature flag on the first/last three days of each month
• Turn on a feature flag during 18:00~23:00 on Feb 14, Dec 24 and 25 every year
• Turn on a feature flag on Dec 31 and Jan 1 every year.
  I think all of them are reasonable scenarios and we should support all of them.
  The current design is the simplest one which can support all of these scenarios.

In fact, the cron expression is the most compact format to describe the recurrence and it is a well-known and standardized format. We can invent some formats to describe the recurrence, but it would be better to use something standardized. (This discussion can be found in the comments of the design doc.)

If you go to see the AKS doc, you will find AKS has a feature called maintenance schedule: https://learn.microsoft.com/en-us/azure/aks/planned-maintenance.
This is a very similar scenario where user can configure some recurring time windows to let AKS cluster to upgrade.
The AKS team designs a quite complicated schema

The AKS team invents a very complex schema to descibe the recurring maintenance time window.

It will look like this.
AKS even does not support configuring this on the Portal. It can only be configured through CLI.

[zhenlan]

I'm not sure I follow what you said here. I did NOT suggest we abandon the usage of cron expression. I am only suggesting we support one cron expression per Time Window filter instead of an array of them. Here is an example.

"EnhancedPipeline": {
    "EnabledFor": [
        {
            "Name": "Microsoft.TimeWindow",
            "Parameters": {
                "Start": "2023-09-06T00:00:00+08:00",
                "RecurrencePattern": "* 18-19 * * Mon-Fri" // 18:00-20:00 on weekdays
            }
        }
    ]
}

[zhiyuanliang-ms]

What if I want the recurrence happen during 18:30`20:30 every day?
You cannot represent 18:30-20:30 in a single cron.

[juniwang]

The scenario when the user want to activate a feature flag during 18:00-20:00 on every weekend and during 18:00-22:00 on holidays requires a list to specify recurring time windows with different durations.

I'm sure we can come up with even more complex scenarios. The question is how realistic they are. Outlook doesn't support infinite recurrence patterns, just one pattern at a time. Without the array, the parameters can be simplified to "Start", "End" and "RecurrencePattern". The UI design doesn't need an add button for more than one recurrence pattern anymore.

Using multiple timewindow filters may not be a good choice, since the feature flag may also use other filters(e.g. targeting), in this case, the "RequirementType" is "All".

We know the "RequirementType" doesn't support all possible logical operations. It was a conscious decision. It's a balance between complexity and the most common scenarios. The choice of that design shouldn't be the reason for us to complicate the design of each feature filter. We have a reasonable workaround. We know there are scenarios that won't be supported out-of-box no matter how. That's where customers should consider custom filters.

Hi @zhenlan, I thinks scenario like 18:30-20:30 everyday is a very common scenario, not something that rarely happens. And current design does not allow us to use one single cron to describe it. Since they are closely tied together, adding two cron expressions makes more sense to me than adding two TimeWindow filters. If we don't support cron array, probablly customers will have to add more Feature filters, which means inceasing size/complexity in another level.

[zhenlan]

@juniwang Yes, we chatted offline, and I understand this now. This limitation imposes challenges on our portal to properly break down 18:30-20:30 into 3 crontabs and reconstruct it back from crontabs to a human-readable time duration. We need to rethink our design choices.

[zhenlan]

0 or 7 is Sunday

Is this from the standard?

[zhiyuanliang-ms]

This is according to the linux man page https://man7.org/linux/man-pages/man5/crontab.5.html
Some cron implementations may not allow 0 to represent Sunday.

[zhiyuanliang-ms]

Update:
We will re-think the design. Current design will inevitably result in the portal UI becoming complex.

[rossgrambo]

We spoke about requiring the start to be within the recurring range.start to range.end window.

If we do that, range.start is not needed. Unless the primary start and end aren't required to be the first one...?

[zhiyuanliang-ms]

@rossgrambo
My thought is that the "Start" and "End" define the first occurence of the time window.
The first time window will provide two information for the recurrence:

1. the start time of the time window 2. the duration of the time window.
   The "Recurrence" only specifies the date/day when the recurrence of the time window will happen.

For me, I think the first time window is unnecessary to follow the recurrence pattern and range.
For example, we have set a time window on 9.22 Friday 18:00-20:00 and let it repeat on every Monday start from 2023-9-31。

My impletation for the validation of the parameter "Recurrence.Range.StartDate" will be like

private static bool TryValidateStartDate(TimeWindowFilterSettings settings, out string paramName, out string reason)
{
    Recurrence recurrence = settings.Recurrence;

    paramName = $"{nameof(settings.Recurrence)}.{nameof(recurrence.Range)}.{nameof(recurrence.Range.StartDate)}";

    reason = null;

    if (recurrence.Range.StartDate == null)
    {
        reason = RequiredParameter;

        return false;
    }

    if (recurrence.Range.StartDate < settings.Start)
    {
        reason = OutOfRange;

        return false;
    }

    return true;
}

[zhenlan]

For example, we have set a time window on 9.22 Friday 18:00-20:00 and let it repeat on every Monday start from 2023-9-31。

For the discussion, in this example (sorry, there is no 9/31 :) so I change it to 9/25)

settings.Start = 9/22 Friday 18:00
settings.End = 9/22 Friday 20:00
recurrence.Range.StartDate = 9/25 Monday 12:00am
recurrence.Range.EndDate = null
recurrence.type = weekly, Monday

IMO, we have two options

1. We use settings.Start/End to calculate the start time and duration and throw away the date part afterwards, so the first occurrence is 9/25 Monday 18:00-20:00
2. The settings.Start/End must be the first occurrence that matches the pattern, so this is treated as invalid input. Portal will show error; runtime will either throw or treat it as disabled.

I don't like treating settings.Start/End as the first occurrence regardless and then calculating the following occurrences (likely still based on option 1 above).

[jimmyca15]

@zhenlan

date ends up being important if you have an event that spans across midnight.

[rossgrambo]

date ends up being important if you have an event that spans across midnight.

It's important to be defined on the field. But if you're extracting duration from it as Zhenlan suggested, you don't need date anymore, as start + duration tells you everything.

But regardless, I think @zhiyuanliang-ms 's version makes the most sense.

1. TimeWindow start and end defines a time when we will return true. (Already the state of things)
2. Completely separately, here's a way to setup recurrence. We will use the start time and end time of the time window.

If we really want to support hourly recurrance then bullet 2 would need to change. But without that, I think that's the cleanest way to do it.

[jimmyca15]

But regardless, I think @zhiyuanliang-ms 's version makes the most sense.
TimeWindow start and end defines a time when we will return true. (Already the state of things)
Completely separately, here's a way to setup recurrence. We will use the start time and end time of the time window.

@rossgrambo that's how I imagine it working.

[zhenlan]

Users shouldn't have a recurring time window that has any occurrence outside the range.

[jimmyca15]

@zhiyuanliang-ms

Zhenlan, Ross, and I discussed this. We came up with the following points.

• We don't need recurrence.range.startDate
  It's already covered by our time window start field. We just mandate that the start date represents the first occurrence.

• start has to be a valid occurrence within the recurrence pattern. After all, it marks the first occurrence.

• Some extra validation we discussed -- The date part of start has to be strictly less than endDate

[zhiyuanliang-ms]

@jimmyca15 Got it.

[zhiyuanliang-ms]

@rossgrambo @zhenlan @jimmyca15 Since we have decided to remove "Recurrence.Range.StartDate", I am wondering whether we still need "Recurrence.Range.RecurrenceTimeZone". We can mandate that the time zone of "Start" will be applied to the recurrence.

Update:
I think the "RecurrenceTimeZone" is necessary since the recurrence pattern does not provide the time zone information.
For fixed time window, we only need two timestamps. For recurring time window, time zone information is essential.

[zhiyuanliang-ms]

I noticed that the current behavior of Portal is that the "Start" and "End" will be converted into GMT format string rather than using the time zone selected by the user. The time zone information will not be provided in "Start" or "End".
It doesn't matter for the fixed time window. But for the recurring time window, time zone matters. So I guess we may still need the "RecurrenceTimeZone".

If the "RecurrenceTimeZone" is specified, it will be applied to the "Start" and then we will check whether the "Start" timestamp is valid for the Recurrence.Pattern under the "RecurrenceTimeZone".
If the "RecurrenceTimeZone" is not specified, the UTC time offset of "Start" will be applied to the recurrence.

Besides, I believe Portal should provide the time zone information in the datetime string by using format like "Sat, 23 Sep 2023 08:00:00 GMT+0800" rather than "Sat, 23 Sep 2023 00:00:00 GMT", since we allow user to select the timezone on the Portal.

If the time zone information is provided in the string for "Start", then we can discard the parameter "Recurrence.Range.RecurrenceTimeZone".

[zhiyuanliang-ms]

I noticed that the current behavior of Portal is that the "Start" and "End" will be converted into GMT format string rather than using the time zone selected by the user. The time zone information will not be provided in "Start" or "End".

I think the current behavior of Portal will break the recurrence pattern.
The recurrence pattern only defines on which day/date the recurrrence will occur, the start time information is provided by the "Start".

If user selects 2023-9-24 6:00-8:00 and time zone +08:00, this time window will be converted into "Sat, 23 Sep 2023 22:00:00 GMT to "Sun, 24 Sep 2023 00:00:00 GMT".
In this case, the "RecurrenceTimeZone" should be a required parameter instead of an optional one.

[zhiyuanliang-ms]

The parameter "FirstDayOfWeek" looks redundant for me. I cannot figure out what it is used for.

Any idea about this?
I plan to remove this parameter.

Update:
I figured out the usage of parameter "FirstDayOfWeek". We should not remove this parameter.

Start = DateTimeOffset.Parse("2023-9-1T00:00:00+08:00"),
End = DateTimeOffset.Parse("2023-9-1T00:00:01+08:00"),
Recurrence = new Recurrence()
{
    Pattern = new RecurrencePattern()
    {
        Type = "Weekly",
        Interval = 2,
        DaysOfWeek = new List<string>(){ "Monday", "Friday" }
    },
    Range = new RecurrenceRange()
    {
        Type = "NoEnd",
    }
}

2023-9-1 is Friday and 2023-9-4 is Monday.
If the "FirstDayOfWeek" is Sunday, then 2023-9-4 will not hit this recurrence pattern, since it is on the next week.
If the "FirstDayOfWeek" is Friday, then 2023-9-4 will hit the pattern, since it is still in the first week.

[rossgrambo]

The parameter "FirstDayOfWeek" looks redundant for me

My understanding is It allows you to pick what day is the first day of the week. If you pass sunday, nothing changes (at least that's what i'm used to). If you pass monday, then the value in index might be affected. For example:

// Lets say the date is Sun, Oct 1st.

DaysOfWeek: "Sunday", "Wednesday"
FirstDayOfWeek: "Monday",
Index: "First"

With this, the "first" week is considered ending on Sun, Oct 1st. Mon, Oct 2nd would be the "second" week. We would never hit "Wednesday" of the "first" week. Europe generally starts weeks on Monday while the US starts weeks on Sunday.

I think we should include it.

In this case, the "RecurrenceTimeZone" should be a required parameter instead of an optional one.

I'm a little lost here. If we use the Start time zone and Sept 23rd would be "converted" to Sept 24th, that's fine. There's no actual conversion. If they use UTC and sept 23rd, that's the start. If they use +08:00, and pick sept 23rd, then that's the start instead. These are different times in UTC.

This means:

"Start": "2023-9-24 6:00 +8:00",
"Recurrance": {
  "dayOfMonth": "25"
}

Would repeat at "Sat, 25 Sep 2023 06:00:00 GMT+0800" / "Sun, 24 Sep 2023 22:00:00 GMT"

Let me know if I'm missing something

[zhiyuanliang-ms]

I'm a little lost here. If we use the Start time zone and Sept 23rd would be "converted" to Sept 24th, that's fine. There's no actual conversion. If they use UTC and sept 23rd, that's the start. If they use +08:00, and pick sept 23rd, then that's the start instead. These are different times in UTC.

Would repeat at "Sat, 25 Sep 2023 06:00:00 GMT+0800" / "Sun, 24 Sep 2023 22:00:00 GMT"
Let me know if I'm missing something

"Sat, 25 Sep 2023 06:00:00 GMT+0800" and "Sun, 24 Sep 2023 22:00:00 GMT" are the same timestamp, they are representing the same physical time.

Currently, no matter what time zone you choose on the Portal, the timestamp will be converted into GMT time(which is under UTC+0:00)
For the fixed time window, the current behavior of Portal will not affect how the filter works. The fixed time window is still the same.

For the recurrence, according to Jimmy's comment, we should add this check for the "Start".

start has to be a valid occurrence within the recurrence pattern. After all, it marks the first occurrence.

It is possible that under UTC+08:00 the "Start" is on Monday and ,when it is converted to GMT time, it becomes Sunday.
The user sets the recurrence Pattern as "Weekly on Monday" The "Start" will not be a valid first occurrence.
The feature management library cannot know the time zone information from the GMT format "Start" and "End" if the "RecurrenceTImeZone" is missing.

[rossgrambo]

I see, so it's a concern for the portal, as it's already doing the conversion, but you're worried the rest of the settings won't be accurate anymore.

I think there's 3 options:

1. Stop the portal from converting to GMT, as we now care more about the timezone
2. Convert the other settings as well. If the GMT conversion changes the date, in the settings, -1/+1 from the daysOfMonth & daysOfWeeks.
   1. We'd have to support "-1" for days like cron
   2. I think there'd be some weirdness on Feb 29th, but haven't thought it through.
3. Add the recurrence time zone. This would be applied to the date and then checked for matching the recurrence.

I'd prefer 1, but we can go with 3 if we're concerned with the impact of 1.

[zhiyuanliang-ms]

I like the option 1. But we need to investigate whether there is a format with the time zone info which can be parsed by spring, dotnet and javascript.

[rossgrambo]

Closing this PR for now in favor of #266