Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(api) Make duration fields available as filters in the /cases/search/ page #2558

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mfonism
Copy link
Contributor

@mfonism mfonism commented Oct 15, 2021

Refs #1923

Without filtering on any duration field

kiwitcms-00-filter-on-no-duration

Filtering on expected duration

kiwitcms-01-filter-on-expected-duration

@mfonism mfonism force-pushed the 2021.10.15/fix-1923 branch from 40e85d2 to 6b557fe Compare October 15, 2021 15:01
Copy link
Member

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Added some comments and some changes requested.

tcms/testcases/templates/testcases/search.html Outdated Show resolved Hide resolved
{{ form.expected_duration }}
</div>
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm. This should allow for an interval inclusion, e.g. search for test cases where setup_duration is between 5 and 10 minutes. See the created field on this page. With the current duration widgets the UI will become rather clunky IMO. I don't have good suggestions though.

Copy link
Member

Choose a reason for hiding this comment

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

See https://ux.stackexchange.com/questions/45656/time-duration-entry-in-webapp-pros-cons-of-various-designs for some hints & inspiration.

The widget itself needs to be as compact as possible because of the limited real estate on the screen, however it needs to allow for selecting either an exact duration (e.g. ==) or a range (from-to). One idea that comes to mind is a more complex JS widget with popups that include the individual bootstrap duration fields. However that could also be a next step b/c it sounds like a lot of work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've looked at the SE page you linked in your comment. I have an idea for the more complex widget you suggested. I'll flesh out the idea on paper and share the sketch with you on Slack.

Copy link
Member

Choose a reason for hiding this comment

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

OK, let's see how it will look like. Just remember - keep it simple as much as possible.

tcms/testcases/static/testcases/js/search.js Show resolved Hide resolved
tcms/testcases/static/testcases/js/search.js Outdated Show resolved Hide resolved
tcms/testcases/static/testcases/js/search.js Outdated Show resolved Hide resolved
if query is None:
query = {}

if "setup_duration" in query:
query["setup_duration"] = parse_duration(query["setup_duration"])
Copy link
Member

Choose a reason for hiding this comment

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

Question: do comparisons (lookups __gt, __lt, etc) against values in seconds work for these fields ?

Also see for hints the __range field lookup, which is however only defined for DateTime fields:
https://docs.djangoproject.com/en/3.2/ref/models/querysets/#range

IDK if it will be any good but we may need to resort to creating our own field lookup around duration fields. I'm not seeing anything out of the box. Still, let's not get ahead of ourselves, maybe this functionality can be implemented in a more straight-forward way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, those sorts of lookups don't work for these fields in this implementation.

One way to make them work would be to transform query in this manner:

for key, val in query.items():
    if not key.startswith(
        ("setup_duration", "testing_duration", "expected_duration")
    ):
        continue

    try:
        duration = parse_duration(val)
    except TypeError:
        # val isn't a string or byte-like object
        # item is probably something like 'setup_duration__isnull=True'
        continue

    if duration is None:
        # parsing duration didn't work, leave the item (key, val) as is
        continue

    query[key] = duration

Copy link
Member

Choose a reason for hiding this comment

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

ATM I'm not sure I like this approach. Let's agree on how the widget portion will work/look like and then we can go back to the API portion and decide what to do.

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.

2 participants