diff --git a/CHANGELOG.md b/CHANGELOG.md index bac7ddb..1b11c74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,12 @@ The format of this file is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and I do try to adhere to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased + +### Fixed + +* `interactive manage-tasks`: a parent task postponed together with its child could be postponed once more in its own turn, and with caldav 3.6 this crashed with a "412 Precondition Failed" error. It is now skipped, with a note. A task changed on the server in the meantime is reloaded before it is postponed. + ## v1.3.0 - 2026-10-23 ### Added diff --git a/plann/commands.py b/plann/commands.py index 0b58fc7..68157f7 100644 --- a/plann/commands.py +++ b/plann/commands.py @@ -511,6 +511,8 @@ def _dismiss_panic(ctx, hours_per_day, lookahead='60d'): click.echo("No need to panic :-)") return + ## a task may be postponed through its child before its own turn + postponed = set() for priority in range(9,0,-1): first_low_pri_tasks = [] other_low_pri_tasks = [] @@ -560,13 +562,13 @@ def _dismiss_panic(ctx, hours_per_day, lookahead='60d'): ## TODO: since tasks with different priority may be affected, it's needed to restart the algorithm _mass_interactive_edit([x['obj'] for x in first_low_pri_tasks], default=f"postpone {default_procrastination_time}") else: - _procrastinate([x['obj'] for x in first_low_pri_tasks], procrastination_time, check_dependent='interactive', err_callback=click.echo, confirm_callback=click.confirm) + _procrastinate([x['obj'] for x in first_low_pri_tasks], procrastination_time, check_dependent='interactive', err_callback=click.echo, confirm_callback=click.confirm, postponed=postponed) if other_low_pri_tasks: - click.echo(f"There are {len(other_low_pri_tasks)} later pri>={priority} tasks selected which should maybe probably be considered to be postponed a bit as well") + click.echo(f"There are {len(other_low_pri_tasks)} later pri>={priority} tasks selected which should maybe be considered to be postponed a bit as well") procrastination_time = click.prompt("Push the due-date for those with ...", default='0h') if procrastination_time not in ('0', '0h', '0m', '0d', 0): - _procrastinate([x['obj'] for x in other_low_pri_tasks], procrastination_time, check_dependent='interactive', err_callback=click.echo, confirm_callback=click.confirm) + _procrastinate([x['obj'] for x in other_low_pri_tasks], procrastination_time, check_dependent='interactive', err_callback=click.echo, confirm_callback=click.confirm, postponed=postponed) def _split_huge_tasks(ctx, threshold='4h', max_lookahead='60d', limit_lookahead=640): _select(ctx=ctx, todo=True, end=f"+{max_lookahead}", limit=limit_lookahead, sort_key=['{DTSTART:?{DUE:?(0000)?}?%F %H:%M:%S}', '{PRIORITY:?0?}']) diff --git a/plann/lib.py b/plann/lib.py index 63b6508..a0cfe42 100644 --- a/plann/lib.py +++ b/plann/lib.py @@ -17,6 +17,7 @@ import click ## TODO - this should be removed, eventually import icalendar from caldav.config import extract_conn_params_from_section +from caldav.lib.error import ETagMismatchError, NotFoundError, ScheduleTagMismatchError, UidConflictError from plann.template import Template from plann.timespec import ( @@ -301,14 +302,23 @@ def _summary(obj): childlike = {'CHILD', 'NEXT', 'FINISHTOSTART'} parentlike = {'PARENT', 'FIRST', 'DEPENDS-ON', 'STARTTOFINISH'} -def _procrastinate(objs, delay, check_dependent="error", with_children=False, with_family=False, with_parent=False, err_callback=print, confirm_callback=lambda x: False, recursivity=0): +def _procrastinate(objs, delay, check_dependent="error", with_children=False, with_family=False, with_parent=False, err_callback=print, confirm_callback=lambda x: False, postponed=None, reload_on_conflict=True, recursivity=0): if delay in ('0', '0s', '0m', '0h', '0d', datetime.timedelta(0)): ## Do nothing! return assert recursivity<16 ## TODO: better error message. Probably we have some kind of relationship loop here. + ## UIDs already postponed. A task may come up again as another + ## object, i.e. a parent fetched by set_due and postponed with its + ## child. Pass the same set to postpone several lists. + if postponed is None: + postponed = set() for x in objs: if not hasattr(x, 'set_due'): continue + uid = x.icalendar_component.get('UID') + if uid and uid in postponed: + err_callback(f"{_summary(x)} was already postponed") + continue if x.icalendar_component.get('STATUS', 'NEEDS-ACTION') == 'COMPLETED': continue if x.icalendar_component.get('RELATED-TO'): @@ -325,15 +335,15 @@ def _procrastinate(objs, delay, check_dependent="error", with_children=False, wi for rel_type in parents_: parents.update(parents_[rel_type]) if parents: - _procrastinate(parents, delay, check_dependent, with_children, with_family, with_parent, err_callback, confirm_callback, recursivity=recursivity+1) + _procrastinate(parents, delay, check_dependent, with_children, with_family, with_parent, err_callback, confirm_callback, postponed=postponed, recursivity=recursivity+1) continue else: - _procrastinate([x], delay, check_dependent, with_children=True, with_family=False, with_parent=False, err_callback=err_callback, confirm_callback=confirm_callback, recursivity=recursivity+1) + _procrastinate([x], delay, check_dependent, with_children=True, with_family=False, with_parent=False, err_callback=err_callback, confirm_callback=confirm_callback, postponed=postponed, recursivity=recursivity+1) continue if with_parent: parents = x.get_relatives(reltypes=parentlike) for rel_type in parents: ## Should only be PARENT as for now. - _procrastinate(parents[rel_type], delay, check_dependent, with_children=True, with_family=False, with_parent=False, err_callback=err_callback, confirm_callback=confirm_callback, recursivity=recursivity+1) + _procrastinate(parents[rel_type], delay, check_dependent, with_children=True, with_family=False, with_parent=False, err_callback=err_callback, confirm_callback=confirm_callback, postponed=postponed, recursivity=recursivity+1) chk_parent = 'return' if check_dependent else False if isinstance(delay, datetime.date): @@ -354,7 +364,7 @@ def _procrastinate(objs, delay, check_dependent="error", with_children=False, wi summary = _summary(i) p = parent.icalendar_component if p.get('STATUS') == 'COMPLETED': - _procrastinate([x], new_due, check_dependent=False, err_callback=err_callback, confirm_callback=confirm_callback, recursivity=recursivity+1) + _procrastinate([x], new_due, check_dependent=False, err_callback=err_callback, confirm_callback=confirm_callback, postponed=postponed, recursivity=recursivity+1) else: p_postponable = check_dependent == "interactive" and p.get('priority', 9)>2 p_auto_postponable = p_postponable and i.get('priority',0) <= p.get('priority', 0) @@ -363,19 +373,38 @@ def _procrastinate(objs, delay, check_dependent="error", with_children=False, wi else: err_callback(f"{summary} could not be postponed due to parent {_summary(p)} with due {_ensure_ts(p['DUE'])} and priority {p.get('priority', 0)}") if p_postponable and (p_auto_postponable or confirm_callback("procrastinate parent?")): - _procrastinate([parent], new_due+max(parent.get_duration()+x.get_duration()+datetime.timedelta(minutes=1), datetime.timedelta(minutes=1)), check_dependent=check_dependent, err_callback=err_callback, confirm_callback=confirm_callback, recursivity=recursivity+1) - _procrastinate([x], new_due, check_dependent=check_dependent, err_callback=err_callback, confirm_callback=confirm_callback, recursivity=recursivity+1) + ## the child needs it moved, even if postponed already + postponed.discard(p.get('UID')) + _procrastinate([parent], new_due+max(parent.get_duration()+x.get_duration()+datetime.timedelta(minutes=1), datetime.timedelta(minutes=1)), check_dependent=check_dependent, err_callback=err_callback, confirm_callback=confirm_callback, postponed=postponed, recursivity=recursivity+1) + _procrastinate([x], new_due, check_dependent=check_dependent, err_callback=err_callback, confirm_callback=confirm_callback, postponed=postponed, recursivity=recursivity+1) elif check_dependent == "return": return parent else: - x.save() + try: + x.save() + except UidConflictError: + raise + except (ETagMismatchError, ScheduleTagMismatchError): + if not reload_on_conflict: + err_callback(f"{_summary(x)} could not be saved, it keeps changing on the server") + continue + err_callback(f"{_summary(x)} was changed on the server, reloading it") + try: + x.load() + except NotFoundError: + err_callback(f"{_summary(x)} was deleted on the server") + continue + _procrastinate([x], delay, check_dependent, with_children, with_family, with_parent, err_callback, confirm_callback, postponed=postponed, reload_on_conflict=False, recursivity=recursivity+1) + continue + if uid: + postponed.add(uid) if with_children: ## TODO: refactor. Make relations into a class. children_ = x.get_relatives(reltypes=childlike) children = set() for rel_type in children_: children.update(children_[rel_type]) - _procrastinate(children, delay, check_dependent, with_children=True, with_family=False, with_parent=False, err_callback=err_callback, confirm_callback=confirm_callback, recursivity=recursivity+1) + _procrastinate(children, delay, check_dependent, with_children=True, with_family=False, with_parent=False, err_callback=err_callback, confirm_callback=confirm_callback, postponed=postponed, recursivity=recursivity+1) def _adjust_ical_relations(obj, relations_wanted={}): """ diff --git a/tests/test_lib.py b/tests/test_lib.py index dee6f19..606583b 100644 --- a/tests/test_lib.py +++ b/tests/test_lib.py @@ -307,6 +307,139 @@ def test_procrastinate_without_relations(): timearg = set_due_mocked.call_args[0][0] assert(timearg.astimezone(utc) == future+timedelta(days=10)) +def _todo(uid, priority=5, related_to=None): + t = Todo() + t.data = todo.replace('19970901T130000Z-123404@host.com', uid).replace('PRIORITY:2', f'PRIORITY:{priority}') + if related_to: + t.icalendar_component.add('RELATED-TO', related_to, parameters={'RELTYPE': 'PARENT'}) + return t + +def test_procrastinate_parent_postponed_by_child(): + """A parent auto-postponed together with its child is fetched + through get_relatives, i.e. as a separate object. When the stale + copy of the parent comes up later in the list, it should not be + postponed (and saved) once more - the server would refuse the + stale copy, and without the precondition it would get postponed + twice.""" + parent_fresh = _todo('parent') + parent_stale = _todo('parent') + child = _todo('child', related_to='parent') + with patch.object(child, 'save'), \ + patch.object(child, 'set_due', side_effect=[parent_fresh, None]), \ + patch.object(parent_fresh, 'save') as fresh_save, \ + patch.object(parent_fresh, 'set_due', return_value=None), \ + patch.object(parent_stale, 'save') as stale_save, \ + patch.object(parent_stale, 'set_due', return_value=None) as stale_set_due: + postponed = set() + _procrastinate([child], '10d', check_dependent='interactive', err_callback=lambda x: None, postponed=postponed) + assert fresh_save.call_count == 1 + ## another call, as from _dismiss_panic, sharing the set + _procrastinate([parent_stale], '10d', check_dependent='interactive', err_callback=lambda x: None, postponed=postponed) + assert stale_set_due.call_count == 0 + assert stale_save.call_count == 0 + +def test_procrastinate_parent_postponed_again_by_child(): + """A parent already postponed still has to move when a child + needs more time""" + parent_fresh = _todo('parent') + child = _todo('child', related_to='parent') + with patch.object(child, 'save'), \ + patch.object(child, 'set_due', side_effect=[parent_fresh, None]), \ + patch.object(parent_fresh, 'save') as fresh_save, \ + patch.object(parent_fresh, 'set_due', return_value=None): + postponed = {'parent'} + _procrastinate([child], '10d', check_dependent='interactive', err_callback=lambda x: None, postponed=postponed) + assert fresh_save.call_count == 1 + assert postponed == {'parent', 'child'} + +def test_procrastinate_parent_postponed_by_child_same_list(): + """As above, in one call with the default set, and the skip is + reported""" + parent_fresh = _todo('parent') + parent_stale = _todo('parent') + child = _todo('child', related_to='parent') + msgs = [] + with patch.object(child, 'save'), \ + patch.object(child, 'set_due', side_effect=[parent_fresh, None]), \ + patch.object(parent_fresh, 'save'), \ + patch.object(parent_fresh, 'set_due', return_value=None), \ + patch.object(parent_stale, 'save') as stale_save: + _procrastinate([child, parent_stale], '10d', check_dependent='interactive', err_callback=msgs.append) + assert stale_save.call_count == 0 + assert any('already postponed' in m for m in msgs) + +def _real_loads(load): + ## icalendar_component calls load(only_if_unloaded=True) + return [c for c in load.call_args_list if not c.kwargs.get('only_if_unloaded')] + +@pytest.mark.parametrize('err', ['ScheduleTagMismatchError', 'ETagMismatchError']) +def test_procrastinate_reloads_on_precondition_failure(err): + """A task changed on the server since it was read is reloaded and + postponed from what the server has""" + from caldav.lib import error + t = _todo('changed') + def load(*args, **kwargs): + if not kwargs.get('only_if_unloaded'): + t.data = t.data.replace('DUE:19970416T045959Z', 'DUE:20530101T120000Z') + with patch.object(t, 'save', side_effect=[getattr(error, err)(), None]) as save, \ + patch.object(t, 'load', side_effect=load) as load_mock, \ + patch.object(t, 'set_due', return_value=None) as set_due: + _procrastinate([t], '10d', err_callback=lambda x: None) + assert len(_real_loads(load_mock)) == 1 + assert save.call_count == 2 + assert set_due.call_count == 2 + ## from the reloaded due, not the stale one + assert set_due.call_args[0][0].astimezone(utc) == datetime(2053, 1, 11, 12, tzinfo=utc) + +def test_procrastinate_precondition_failure_twice(): + """A 412 that persists after reloading is reported, not retried + forever""" + from caldav.lib import error + t = _todo('changed') + msgs = [] + with patch.object(t, 'save', side_effect=error.ScheduleTagMismatchError()) as save, \ + patch.object(t, 'load') as load, \ + patch.object(t, 'set_due', return_value=None): + _procrastinate([t], '10d', err_callback=msgs.append) + assert len(_real_loads(load)) == 1 + assert save.call_count == 2 + assert any('could not' in m for m in msgs) + +def test_procrastinate_uid_conflict_not_retried(): + from caldav.lib import error + t = _todo('changed') + with patch.object(t, 'save', side_effect=error.UidConflictScheduleTagMismatchError()), \ + patch.object(t, 'load') as load, \ + patch.object(t, 'set_due', return_value=None): + with pytest.raises(error.UidConflictError): + _procrastinate([t], '10d', err_callback=lambda x: None) + assert _real_loads(load) == [] + +def test_procrastinate_deleted_meanwhile(): + """A task deleted on the server since it was read is skipped""" + from caldav.lib import error + t = _todo('gone') + msgs = [] + def load(*args, **kwargs): + if not kwargs.get('only_if_unloaded'): + raise error.NotFoundError() + with patch.object(t, 'save', side_effect=error.ETagMismatchError()), \ + patch.object(t, 'load', side_effect=load), \ + patch.object(t, 'set_due', return_value=None): + _procrastinate([t], '10d', err_callback=msgs.append) + assert any('deleted' in m for m in msgs) + +def test_procrastinate_without_uid(): + """Tasks without a UID are not taken for one another""" + a = _todo('a') + b = _todo('b') + for t in (a, b): + t.icalendar_component.pop('UID') + with patch.object(a, 'save'), patch.object(a, 'set_due', return_value=None), \ + patch.object(b, 'save') as b_save, patch.object(b, 'set_due', return_value=None): + _procrastinate([a, b], '10d') + assert b_save.call_count == 1 + def test_adjust_ical_relations(): t = Todo() t.data = todo