Repository: ansible/awx
File: awx/main/models/schedules.py (Python)

Code before:
from django.utils.timezone import now, make_aware
from django.utils.translation import gettext_lazy as _

# AWX
from awx.api.versioning import reverse
from awx.main.fields import OrderedManyToManyField
from awx.main.models.base import PrimordialModel
from awx.main.models.jobs import LaunchTimeConfig
from awx.main.utils import ignore_inventory_computed_fields
from awx.main.consumers import emit_channel_notification

import pytz


logger = logging.getLogger('awx.main.models.schedule')

__all__ = ['Schedule']


UTC_TIMEZONES = {x: tzutc() for x in dateutil.parser.parserinfo().UTCZONE}


def _check_valid_tzid(rrules):
    for r in rrules:
        if r._dtstart and r._dtstart.tzinfo is None:
            raise ValueError('A valid TZID must be provided (e.g., America/New_York)')


def _fast_forward_rrules(rrules, ref_dt=None):
    for i, rule in enumerate(rrules):
        rrules[i] = _fast_forward_rrule(rule, ref_dt=ref_dt)
    return rrules


def _fast_forward_rrule(rrule, ref_dt=None):
    '''
    Utility to fast forward an rrule, maintaining consistency in the resulting
    occurrences.

    Uses the .replace() method to update the rrule with a newer dtstart
    The operation ensures that the original occurrences (based on the original dtstart)
    will match the occurrences after changing the dtstart.

    Returns a new rrule with a new dtstart
    '''
    if rrule._freq not in {dateutil.rrule.HOURLY, dateutil.rrule.MINUTELY}:
        return rrule

    if ref_dt is None:
        ref_dt = now()


Review comment (bug):
Since this is not doing `if any(): raise` but has an explicit top-level loop, I think it's important to mention what actually is wrong and where.

Could you add the specific rrule details into the message? And maybe spell out the abbreviations? As in, change “TZID” to “time zone identifier”, for example.

But also, perhaps it'd be better to go through all the rules and then add details on the batch of broken rules?

I'm thinking something along the lines of
```suggestion
def _assert_timezone_id_is_valid(rrules) -> None:
    broken_rrules = [
        rrule for rrule in rrules
        if r._dtstart and r._dtstart.tzinfo is None
    ]
    if not broken_rrules:
        return
    raise ValueError(
        'The following rrules do not a timezone set: '
        f'{broken_rrules}. Please, set it for each '
        '(e.g., America/New_York).',
    ) from None
```

Code after:
from django.utils.timezone import now, make_aware
from django.utils.translation import gettext_lazy as _

# AWX
from awx.api.versioning import reverse
from awx.main.fields import OrderedManyToManyField
from awx.main.models.base import PrimordialModel
from awx.main.models.jobs import LaunchTimeConfig
from awx.main.utils import ignore_inventory_computed_fields
from awx.main.consumers import emit_channel_notification

import pytz


logger = logging.getLogger('awx.main.models.schedule')

__all__ = ['Schedule']


UTC_TIMEZONES = {x: tzutc() for x in dateutil.parser.parserinfo().UTCZONE}


def _assert_timezone_id_is_valid(rrules) -> None:
    broken_rrules = [str(rrule) for rrule in rrules if rrule._dtstart and rrule._dtstart.tzinfo is None]
    if not broken_rrules:
        return
    raise ValueError(
        f'A valid TZID must be provided (e.g., America/New_York). Invalid: {broken_rrules}',
    ) from None


def _fast_forward_rrules(rrules, ref_dt=None):
    for i, rule in enumerate(rrules):
        rrules[i] = _fast_forward_rrule(rule, ref_dt=ref_dt)
    return rrules


def _fast_forward_rrule(rrule, ref_dt=None):
    '''
    Utility to fast forward an rrule, maintaining consistency in the resulting
    occurrences.

    Uses the .replace() method to update the rrule with a newer dtstart
    The operation ensures that the original occurrences (based on the original dtstart)
    will match the occurrences after changing the dtstart.

    Returns a new rrule with a new dtstart
    '''
    if rrule._freq not in {dateutil.rrule.HOURLY, dateutil.rrule.MINUTELY}:
        return rrule