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

Feature/fwf 4371 Backend support different filter types #2596

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

auslin-aot
Copy link
Contributor

@auslin-aot auslin-aot commented Feb 25, 2025

Issue Tracking

JIRA:
Issue Type: BUG/ FEATURE
https://aottech.atlassian.net/browse/FWF-4371

Changes

  • Added column filter_type with values TASK (default value), ATTRIBUTE, DATE
  • Updated existing filter to type TASK
  • Added a new column called parent_filter_id
  • Changes are done in the get filter by id API to return attribute filters corresponding to the task filter

Screenshots

image
image

Checklist

  • Updated changelog
  • Added meaningful title for pull request

@auslin-aot auslin-aot marked this pull request as ready for review February 25, 2025 11:11
@auslin-aot auslin-aot requested review from a team as code owners February 25, 2025 11:11
@@ -121,6 +158,8 @@ def get_user_filters(**kwargs):
filter_item["variables"] += [
var for var in default_variables if var not in filter_item["variables"]
]
# Return attribute filters for task filters
FilterService.set_attribute_filters(filter_item, attribute_filters)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this here ?
Can't we have 2 endpoints which I think will be easy to handle on the UI ?
Please note that there are other changes coming in to the picture on show/hide and re-ordering. So I feel it would be better to keep each filter as separate response.
Endpoints would be as simple as /filters/TASK
/filters/ATTRIBUTE?parenFilterId=<>

What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The approach what auslin has followed is to quickly load the initial data when user comes to review screen.

  1. Suppose if we have more number of attribute filters and task filters, if we go with task api which will also have all attributes inside it , TIME TO GET THE RESPONSE WILL BE MORE and only after the response is received , the task list will be refreshed and shown.

  2. If we go with initially loading the TASK FILTER list , and ATTRIBUTE FILTER list after that , Task listing will have to wait for 2 APIs ( synchoronous ) to complete inorder to refresh and show.

Inorder to mitigate the above 2 issues, we thought of returning the TASK filter initially with either the first ATTRIBUTE filter alone in an array , or already selected 1 ATTRIBUTE filter as part of TASK filter and parallely we will call the full ATTRIBUTE filter endpoint so that user wont feel the delay in screen. By the time user plans to change the attibute filter, the ATTRIBUTE filter list will be available to frontend

We will definitely need attibute listing GET endpoint as a separate one so that if we change the TASK filter, updated attributes can be retrieved. Any other use cases like show/hide and re-ordering might need this endpoint.

@sumesh-aot Please share your thoughts
cc @auslin-aot

Copy link
Member

Choose a reason for hiding this comment

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

Understood.
Adding just one filter to the list is misrepresentation of the resource.

  • For 7.1.0 : we will only default the main filter
  • Keep the attribute filter selection as empty on first load
  • Let the user select the attribute filter based on their need
  • On change of task filter, save it as the default filter
  • Keep the main filter, attribute filter stored in state, so that if the user navigates on the same session and comes back they will see the last worked state

If we go with this, we wouldn't need the attributes filter to load initially right ?

Copy link
Member

Choose a reason for hiding this comment

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

Below are the agreed scope for 7.1.0

  • User can save TASK filter and ATTRIBUTES filter
  • Date filter cannot be saved
  • When a reviewer selects the TASK filter, we will save it as the default filter for the user
  • We won't save the ATTRIBUTES filter on the selection
  • Numbers displayed on the label would be the total number of tasks under TASK filter
  • By default only TASK filter will be displayed, user will need to select ATTRIBUTES filter and put in dates if needed
  • On same session, state will be maintained so that user can navigate and come back to the previous page

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification @sumesh-aot We will separate task and attribute filter and do according to the requirements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed attributeFilters from the response of filter list API

@shuhaib-aot shuhaib-aot added enhancement New feature or request DoNotMerge labels Mar 4, 2025
@auslin-aot auslin-aot marked this pull request as draft March 4, 2025 05:00
@auslin-aot auslin-aot force-pushed the feature/fwf-4371-backend-support-different-filter-types branch from d742f06 to bc8383c Compare March 4, 2025 06:27
@auslin-aot auslin-aot marked this pull request as ready for review March 4, 2025 07:10
Copy link

sonarqubecloud bot commented Mar 4, 2025

@auslin-aot auslin-aot requested a review from sumesh-aot March 5, 2025 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants