Repository: pimutils/todoman
Pull request #229 (2017-04-01 04:14:15)

Allow fine-grained filtering by status
This replaces filtering via `--all` and `--done-only` with fine-grained
status filtering. Previous results can be obtained with `status=ANY` and
`status=COMPLETED`.


Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index a1410da..efd5c6b 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -7,6 +7,9 @@ releases, in reverse chronological order.
 v3.1.0
 ------
 
+* Replace ``--all`` and ``--done-only`` with  ``--status``, which allows
+  fine-grained status filtering. Use ``--status ANY`` or ``--status COMPLETED``
+  to obtain the same results as the previous flags.
 * Rename ``--today`` flag to ``--startable``.
 * Illegal start dates (eg: start dates that are not before the due date) are
   ignored and are removed when saving an edited todo.
diff --git a/todoman/cli.py b/todoman/cli.py
index 7aa6275..c847fca 100644
--- a/todoman/cli.py
+++ b/todoman/cli.py
@@ -97,10 +97,7 @@ def _validate_start_date_param(ctx, param, val):
 
 def _validate_startable_param(ctx, param, val):
     ctx = ctx.find_object(AppContext)
-    if val is not None:
-        return val
-    else:
-        return ctx.config['main']['startable']
+    return val or ctx.config['main']['startable']
 
 
 def _validate_todos(ctx, param, val):
@@ -121,6 +118,28 @@ def _sort_callback(ctx, param, val):
     return fields
 
 
+def validate_status(ctx=None, param=None, val=None):
+    # The default command doesn't run callbacks as expected, so it needs to
+    # specify the callback'd type. When `list` is called explicitly, this
+    # callback *IS* run, so we need to handle that edge case:
+    if not isinstance(val, str):
+        return val
+
+    statuses = val.upper().split(',')
+
+    if 'ANY' in statuses:
+        return Todo.VALID_STATUSES
+
+    for status in statuses:
+        if status not in Todo.VALID_STATUSES:
+            raise click.BadParameter(
+                'Invalid status, "{}", statuses must be one of "{}", or "ANY"'
+                .format(status, ', '.join(Todo.VALID_STATUSES))
+            )
+
+    return statuses
+
+
 def _todo_property_options(command):
     click.option('--location', help=('The location where '
                  'this todo takes place.'))(command)
@@ -426,7 +445,6 @@ def move(ctx, list, ids):
 
 @cli.command()
 @pass_ctx
-@click.option('--all', '-a', is_flag=True, help='Also show finished tasks.')
 @click.argument('lists', nargs=-1, callback=_validate_lists_param)
 @click.option('--location', help='Only show tasks with location containg TEXT')
 @click.option('--category', help='Only show tasks with category containg TEXT')
@@ -436,26 +454,30 @@ def move(ctx, list, ids):
 @click.option('--reverse/--no-reverse', default=True,
               help='Sort tasks in reverse order (see --sort). '
               'Defaults to true.')
-@click.option('--due', default=None, help='Only show tasks due in DUE hours',
-              type=int)
+@click.option('--due', default=None, help='Only show tasks due in INTEGER '
+              'hours', type=int)
 @click.option('--priority', default=None, help='Only show tasks with'
-              ' priority at least as high as the specified one', type=str,
-              callback=_validate_priority_param)
-@click.option('--done-only', default=False, is_flag=True,
-              help='Only show finished tasks')
+              ' priority at least as high as TEXT (low, medium or high).',
+              type=str, callback=_validate_priority_param)
 @click.option('--start', default=None, callback=_validate_start_date_param,
               nargs=2, help='Only shows tasks before/after given DATE')
 @click.option('--startable', default=None, is_flag=True,
               callback=_validate_startable_param, help='Show only todos which '
-              'should can be started today (eg: start time is not in the '
+              'should can be started today (i.e.: start time is not in the '
               'future).')
+@click.option('--status', '-s', default=['NEEDS-ACTION', 'IN-PROCESS'],
+              callback=validate_status, help='Show only todos with the '
+              'provided comma-separated statuses. Valid statuses are '
+              '"NEEDS-ACTION", "CANCELLED", "COMPLETED", "IN-PROCESS" or "ANY"'
+              )
 @catch_errors
 def list(ctx, **kwargs):
     """
-    List unfinished tasks.
+    List tasks. Filters any completed or cancelled tasks by default.
 
-    If no arguments are provided, all lists will be displayed. Otherwise, only
-    todos for the specified list will be displayed.
+    If no arguments are provided, all lists will be displayed, and only
+    incomplete tasks are show. Otherwise, only todos for the specified list
+    will be displayed.
 
     eg:
       \b
@@ -463,6 +485,9 @@ def list(ctx, **kwargs):
       - `todo list work' shows all unfinished tasks from the list `work`.
 
     This is the default action when running `todo'.
+
+    The following commands can further filter shown todos, or include those
+    omited by default:
     """
     todos = ctx.db.todos(**kwargs)
     click.echo(ctx.formatter.compact_multiple(todos))
diff --git a/todoman/model.py b/todoman/model.py
index b66c677..12f7a33 100644
--- a/todoman/model.py
+++ b/todoman/model.py
@@ -166,6 +166,13 @@ class Todo:
         STRING_FIELDS
     )
 
+    VALID_STATUSES = (
+        "CANCELLED",
+        "COMPLETED",
+        "IN-PROCESS",
+        "NEEDS-ACTION",
+    )
+
     def __setattr__(self, name, value):
         # Avoids accidentally setting a field to None when that's not a valid
         # attribute.
@@ -532,7 +539,7 @@ class Cache:
             self._serialize_datetime(todo, 'completed'),
             todo.get('percent-complete', None),
             self._serialize_datetime(todo, 'dtstamp'),
-            todo.get('status', None),
+            todo.get('status', 'NEEDS-ACTION'),
             todo.get('description', None),
             todo.get('location', None),
             todo.get('categories', None),
@@ -553,9 +560,9 @@ class Cache:
 
         return rv
 
-    def todos(self, all=False, lists=[], priority=None, location='',
-              category='', grep='', sort=[], reverse=True, due=None,
-              done_only=None, start=None, startable=False):
+    def todos(self, lists=[], priority=None, location='', category='', grep='',
+              sort=[], reverse=True, due=None, start=None, startable=False,
+              status=['NEEDS-ACTION', 'IN-PROCESS']):
         """
         Returns filtered cached todos, in a specified order.
 
@@ -566,7 +573,6 @@ class Cache:
             due
             -created_at
 
-        :param bool all: If true, also return completed todos.
         :param list lists: Only return todos for these lists.
         :param str location: Only return todos with a location containing this
             string.
@@ -579,23 +585,22 @@ class Cache:
         :param int due: Return only todos due within ``due`` hours.
         :param str priority: Only return todos with priority at least as
             high as specified.
-        :param bool done_only: If true, return done tasks, else incomplete
-            tasks
-        :param start: Return only todos before/after ``start`` date
+        :param tuple(bool, datetime) start: Return only todos before/after
+            ``start`` date
+        :param list(str) status: Return only todos with any of the given
+            statuses.
         :return: A sorted, filtered list of todos.
         :rtype: generator
         """
         extra_where = []
         params = []
 
-        if not all:
-            # XXX: Duplicated logic of Todo.is_completed
-            if done_only:
-                extra_where.append('AND status == "COMPLETED"')
-            else:
-                extra_where.append('AND completed_at is NULL '
-                                   'AND status IS NOT "CANCELLED" '
-                                   'AND status IS NOT "COMPLETED"')
+        if 'ANY' not in status:
+            extra_where.append(
+                'AND status IN ({})'.format(', '.join(['?'] * len(status)))
+            )
+            params.extend(s.upper() for s in status)
+
         if lists:
             lists = [l.name if isinstance(l, List) else l for l in lists]
             q = ', '.join(['?'] * len(lists))
@@ -659,6 +664,10 @@ class Cache:
                WHERE todos.file_path = files.path {}
             ORDER BY {}
         '''.format(' '.join(extra_where), order,)
+
+        logger.debug(query)
+        logger.debug(params)
+
         result = self._conn.execute(query, params)
 
         seen_paths = set()
@@ -887,7 +896,7 @@ class Database:
         os.remove(path)
 
     def flush(self):
-        for todo in self.todos(all=True):
+        for todo in self.todos(status=['ANY']):
             if todo.is_completed:
                 yield todo
                 self.delete(todo)


Test patch:
diff --git a/tests/test_cli.py b/tests/test_cli.py
index e9bb573..289d2ff 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -3,6 +3,7 @@ import sys
 from os.path import isdir
 from unittest.mock import patch
 
+import click
 import hypothesis.strategies as st
 import pytest
 from dateutil.tz import tzlocal
@@ -273,7 +274,7 @@ def test_default_due2(tmpdir, runner, create, todos):
     r = runner.invoke(cli, ['new', '-ldefault', '-d', 'one hour', 'ccc'])
     assert not r.exception
 
-    todos = {t.summary: t for t in todos(all=True)}
+    todos = {t.summary: t for t in todos(status='ANY')}
     assert todos['aaa'].due.date() == todos['bbb'].due.date()
     assert todos['ccc'].due == todos['bbb'].due - datetime.timedelta(hours=23)
 
@@ -388,7 +389,7 @@ def test_color_due_dates(tmpdir, runner, create, hours):
     create(
         'test.ics',
         'SUMMARY:aaa\n'
-        'STATUS:IN-PROGRESS\n'
+        'STATUS:IN-PROCESS\n'
         'DUE;VALUE=DATE-TIME;TZID=ART:{}\n'
         .format(due.strftime('%Y%m%dT%H%M%S'))
     )
@@ -469,7 +470,7 @@ def test_edit(runner, default_database, todos):
     assert not result.exception
     assert '2017-02-01' in result.output
 
-    todo = next(todos(all=True))
+    todo = next(todos(status='ANY'))
     assert todo.due == datetime.datetime(2017, 2, 1, tzinfo=tzlocal())
     assert todo.summary == 'Eat paint'
 
@@ -577,7 +578,7 @@ def test_sort_mixed_timezones(runner, create):
         'DUE;VALUE=DATE-TIME;TZID=HST:20170304T080000\n'  # 1800 UTC
     )
 
-    result = runner.invoke(cli, ['list', '--all'])
+    result = runner.invoke(cli, ['list', '--status', 'ANY'])
     assert not result.exception
     output = result.output.strip()
     assert len(output.splitlines()) == 2
@@ -724,7 +725,7 @@ def test_done(runner, todo_factory, todos):
     result = runner.invoke(cli, ['done', '1'])
     assert not result.exception
 
-    todo = next(todos(all=True))
+    todo = next(todos(status='ANY'))
     assert todo.percent_complete == 100
     assert todo.is_completed is True
 
@@ -739,7 +740,7 @@ def test_cancel(runner, todo_factory, todos):
     result = runner.invoke(cli, ['cancel', '1'])
     assert not result.exception
 
-    todo = next(todos(all=True))
+    todo = next(todos(status='ANY'))
     assert todo.status == 'CANCELLED'
 
 
@@ -775,3 +776,50 @@ def test_no_repl(runner):
     assert 'repl' not in result.output
     assert 'shell' not in result.output
     assert 'Start an interactive shell.' not in result.output
+
+
+def test_status_validation():
+    from todoman import cli
+
+    @given(statuses=st.lists(
+        st.sampled_from(Todo.VALID_STATUSES + ('ANY',)),
+        min_size=1,
+        max_size=5,
+        unique=True
+    ))
+    def run_test(statuses):
+        validated = cli.validate_status(val=','.join(statuses))
+
+        if 'ANY' in statuses:
+            assert len(validated) == 4
+        else:
+            assert len(validated) == len(statuses)
+
+        for status in validated:
+            assert status in Todo.VALID_STATUSES
+
+    run_test()
+
+
+def test_bad_status_validation():
+    from todoman import cli
+    with pytest.raises(click.BadParameter):
+        cli.validate_status(val='INVALID')
+
+    with pytest.raises(click.BadParameter):
+        cli.validate_status(val='IN-PROGRESS')
+
+
+def test_status_filtering(runner, todo_factory):
+    todo_factory(summary='one', status='CANCELLED')
+    todo_factory(summary='two')
+
+    result = runner.invoke(cli, ['list', '--status', 'cancelled'])
+    assert not result.exception
+    assert len(result.output.splitlines()) == 1
+    assert 'one 'in result.output
+
+    result = runner.invoke(cli, ['list', '--status', 'NEEDS-action'])
+    assert not result.exception
+    assert len(result.output.splitlines()) == 1
+    assert 'two' in result.output
diff --git a/tests/test_filtering.py b/tests/test_filtering.py
index 4c8eee6..2e93df9 100644
--- a/tests/test_filtering.py
+++ b/tests/test_filtering.py
@@ -4,27 +4,6 @@ from todoman.cli import cli
 from todoman.model import Database, Todo
 
 
-def test_all(tmpdir, runner, create):
-    result = runner.invoke(cli, ['list'], catch_exceptions=False)
-    assert not result.exception
-    assert not result.output.strip()
-
-    create(
-        'one.ics',
-        'SUMMARY:haha\n'
-    )
-    create(
-        'two.ics',
-        'SUMMARY:hoho\n'
-        'PERCENT-COMPLETE:100\n'
-        'STATUS:COMPLETED\n'
-    )
-    result = runner.invoke(cli, ['list', '--all'])
-    assert not result.exception
-    assert 'haha' in result.output
-    assert 'hoho' in result.output
-
-
 def test_priority(tmpdir, runner, create):
     result = runner.invoke(cli, ['list'], catch_exceptions=False)
     assert not result.exception
@@ -108,27 +87,6 @@ def test_location(tmpdir, runner, create):
     assert 'harhar' not in result.output
 
 
-def test_done_only(tmpdir, runner, create):
-    result = runner.invoke(cli, ['list'], catch_exceptions=False)
-    assert not result.exception
-    assert not result.output.strip()
-
-    create(
-        'one.ics',
-        'SUMMARY:haha\n'
-    )
-    create(
-        'two.ics',
-        'SUMMARY:hoho\n'
-        'PERCENT-COMPLETE:100\n'
-        'STATUS:COMPLETED\n'
-    )
-    result = runner.invoke(cli, ['list', '--done-only'])
-    assert not result.exception
-    assert 'haha' not in result.output
-    assert 'hoho' in result.output
-
-
 def test_category(tmpdir, runner, create):
     result = runner.invoke(cli, ['list'], catch_exceptions=False)
     assert not result.exception
@@ -300,3 +258,29 @@ def test_filtering_start(tmpdir, runner, todo_factory):
     assert 'hoho' not in result.output
     assert 'hihi' not in result.output
     assert 'huhu' not in result.output
+
+
+def test_statuses(todo_factory, todos):
+    cancelled = todo_factory(status='CANCELLED').uid
+    completed = todo_factory(status='COMPLETED').uid
+    in_process = todo_factory(status='IN-PROCESS').uid
+    needs_action = todo_factory(status='NEEDS-ACTION').uid
+    no_status = todo_factory(status='NEEDS-ACTION').uid
+
+    all_todos = set(todos(status=['ANY']))
+    cancelled_todos = set(todos(status=['CANCELLED']))
+    completed_todos = set(todos(status=['COMPLETED']))
+    in_process_todos = set(todos(status=['IN-PROCESS']))
+    needs_action_todos = set(todos(status=['NEEDS-ACTION']))
+
+    assert {t.uid for t in all_todos} == {
+        cancelled,
+        completed,
+        in_process,
+        needs_action,
+        no_status
+    }
+    assert {t.uid for t in cancelled_todos} == {cancelled}
+    assert {t.uid for t in completed_todos} == {completed}
+    assert {t.uid for t in in_process_todos} == {in_process}
+    assert {t.uid for t in needs_action_todos} == {needs_action, no_status}
diff --git a/tests/test_formatter.py b/tests/test_formatter.py
index a806cfe..74693cc 100644
--- a/tests/test_formatter.py
+++ b/tests/test_formatter.py
@@ -23,7 +23,7 @@ def test_humanized_date(runner, create, interval, now_for_tz, tz):
         .format(tz, due.strftime('%Y%m%dT%H%M%S'))
     )
 
-    result = runner.invoke(cli, ['--humanize', 'list', '--all'])
+    result = runner.invoke(cli, ['--humanize', 'list', '--status', 'ANY'])
     assert not result.exception
     assert expected in result.output
 
diff --git a/tests/test_model.py b/tests/test_model.py
index 4c0f61d..7ac59b2 100644
--- a/tests/test_model.py
+++ b/tests/test_model.py
@@ -301,7 +301,7 @@ def test_hide_cancelled(todos, todo_factory):
     todo_factory(status='CANCELLED')
 
     assert len(list(todos())) == 0
-    assert len(list(todos(all=True))) == 1
+    assert len(list(todos(status='ANY'))) == 1
 
 
 def test_illegal_start_suppression(create, default_database, todos):
@@ -314,3 +314,12 @@ def test_illegal_start_suppression(create, default_database, todos):
     todo = next(todos())
     assert todo.start is None
     assert todo.due == datetime(2017, 3, 31, 12, tzinfo=tzoffset(None, 7200))
+
+
+def test_default_status(create, todos):
+    create(
+        'test.ics',
+        'SUMMARY:Finish all these status tests\n'
+    )
+    todo = next(todos())
+    assert todo.status == 'NEEDS-ACTION'
diff --git a/tests/test_porcelain.py b/tests/test_porcelain.py
index a3d91f4..8a3e210 100644
--- a/tests/test_porcelain.py
+++ b/tests/test_porcelain.py
@@ -15,7 +15,7 @@ def test_list_all(tmpdir, runner, create):
         'DUE;VALUE=DATE-TIME;TZID=CET:20160102T000000\n'
         'PERCENT-COMPLETE:26\n'
     )
-    result = runner.invoke(cli, ['--porcelain', 'list', '--all'])
+    result = runner.invoke(cli, ['--porcelain', 'list', '--status', 'ANY'])
 
     expected = [{
         'completed': True,