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

feat(recordings): Add search filters to target recording tables #486

Merged
merged 25 commits into from
Jul 28, 2022

Conversation

jan-law
Copy link
Contributor

@jan-law jan-law commented Jul 21, 2022

Fixes #414

Filter recordings by name, state, labels, duration, and start time. When a filter is selected, only the recordings matching all the filter criteria will be visible in the active or target-specific archived recordings table. If multiple queries for the same filter category are specified, eg state: RUNNING, state: STOPPED, the results will show recordings matching either of the queries, eg any recording that is either RUNNING or STOPPED.

@jan-law jan-law added feat New feature or request needs-documentation labels Jul 21, 2022
@jan-law
Copy link
Contributor Author

jan-law commented Jul 21, 2022

I had a few questions about the filter design:

  1. Are there any plans to merge the Active recordings table and Archived recordings table for a selected target? As of now, I've programmed the recording Name filter to auto-complete with both active and archived recording names for the selected target and reusing the same filter component for both tables.

Before/after a datetime would also be nice.

  1. Did you mean 'show me all recordings started within the specified datetime range' or 'show me all recordings that were running during the specified datetime range'? If it's the latter, I'd need to determine if a manually stopped continuous recording was running at a certain time, because the duration field is 0 and there's no stopTime field.

  2. For the date/time range, hours and minutes selectors add precision but also add more clutter to the UI. How precise of a time range would users find most helpful?

@andrewazores
Copy link
Member

Are there any plans to merge the Active recordings table and Archived recordings table for a selected target? As of now, I've programmed the recording Name filter to auto-complete with both active and archived recording names for the selected target and reusing the same filter component for both tables.

Those two tables might get merged eventually, but I wouldn't say there are any "plans" to do so.

Did you mean 'show me all recordings started within the specified datetime range' or 'show me all recordings that were running during the specified datetime range'?

I didn't necessarily mean a [start, end] timestamp range provided by the user, though that would eventually be nice too. I was just thinking a "start time before X" filter and a "start time after X" filter.

If it's the latter, I'd need to determine if a manually stopped continuous recording was running at a certain time, because the duration field is 0 and there's no stopTime field.

Don't worry about that for now. That's the kind of thing we should be tracking in a proper database for recordings, so we can have a model for the recording other than the metadata supplied to us by the target JVM. Then we can update it ourselves when we know that we explicitly stopped it.

For the date/time range, hours and minutes selectors add precision but also add more clutter to the UI. How precise of a time range would users find most helpful?

I think what I see in the branch now is fine. The calendar and clock selectors that appear let me select from dropdowns with reasonable precision, and for extra precision I can type in a hh:mm string. Being able to type in hh:mm:ss might be good too, but no need for there to be any to-the-second selections in the dropdown.

@jan-law
Copy link
Contributor Author

jan-law commented Jul 28, 2022

Being able to type in hh:mm:ss might be good too

If you want to include seconds in the TimePicker, we'd need to upgrade patternfly to the 2022-02-22 release to use the includeSeconds prop. I'll leave the filter as hh:mm for now.

@jan-law
Copy link
Contributor Author

jan-law commented Jul 28, 2022

This PR is ready for review. If I have enough time leftover before the end of my internship, I'll add recording filter unit tests in a separate PR.

@jan-law jan-law requested a review from andrewazores July 28, 2022 17:07
@jan-law jan-law marked this pull request as ready for review July 28, 2022 17:08
@andrewazores
Copy link
Member

andrewazores commented Jul 28, 2022

One seeming bug found in manual testing. If I choose the StartedAfterDate filter, select a date from the calendar picker, and then click the search magnifying glass icon, it actually applies a StartedBeforeDate filter with the chosen date. Otherwise this looks and feels really good!

@andrewazores
Copy link
Member

Implementation looks good.

If I have enough time leftover before the end of my internship

:-(

I'll add recording filter unit tests in a separate PR.

Please file a follow-up issue after this is merged so it isn't forgotten. If you have time to take it on then great, otherwise somebody else can pick it up later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request needs-documentation
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Search/filters in Recordings tables
3 participants