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

Dynamic filtering #24

Closed
wants to merge 13 commits into from
Closed

Dynamic filtering #24

wants to merge 13 commits into from

Conversation

findsah
Copy link

@findsah findsah commented Jan 4, 2024

filter component is now similar to all other components , comprehensive unit tests for robust functionality. unit test similar to all existing unit tests in the code . filter linked with form components .Improve rendering logic for enhanced flexibility

…. Use the filter in data fetching methods, allowing customization based on user-selected filter choices. to Improve user interaction and data relevance
…sts add ,dynamic addition of filter fields based on configuration, comprehensive unit tests for robust functionality
…sts add ,dynamic addition of filter fields based on configuration, comprehensive unit tests for robust functionality

return reverse("dashboards:filter_component", args=args)

def get_filter_form(self) -> Type[FilterSet]:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be get_filterset

Copy link
Author

Choose a reason for hiding this comment

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

changed as per feedback

model: Optional[Type[models.Model]] = None
method: Literal["get", "post"] = "get"
submit_url: Optional[str] = None
filter_fields: Optional[Dict[str, Any]] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it makes more sense to just store the filter set class here rather than an arbitrary dict, this would mean all fields wouldn't need to be created as CharFilter

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably also need to validate that the filter is set and raise a ConfigurationError if not

Copy link
Author

Choose a reason for hiding this comment

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

have updated it

class Meta(ModelDataMixin.Meta, PlotlyChartSerializer.Meta):
pass
filter_component = Filter # Added the Filter component here
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to set a default here, it should just be a type annotation and default to None as many use cases wont want a filter on a chart

Copy link
Contributor

Choose a reason for hiding this comment

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

It should also be the FilterSet that gets passed into the filter component rather than the filter component itself

Copy link
Author

Choose a reason for hiding this comment

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

removed from form

@@ -77,6 +80,13 @@ def get_form(self, request: HttpRequest = None) -> DashboardForm:
data = request.GET

form = self.form(data=data)

# Create and apply the GenericFilter if filter_data and filter_fields are provided
if self.filter_data and self.filter_fields and self.model:
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be misunderstanding something but why are we applying a filter here, the form shouldn't be doing any filtering right? it will just be providing the ability to submit data?

Copy link
Author

Choose a reason for hiding this comment

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

filtering removed from form

1. Import GenericFilter:


from dashboards.component.filters import GenericFilter
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this out of date? GenericFilter doesn't exist

Copy link
Author

Choose a reason for hiding this comment

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

updated the documentation it was due to it being implemented this way before

"USER": self.DATABASE_USER,
"PASSWORD": self.DATABASE_PASSWORD,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably makes sense for the demo to use sqlite, just need to make sure we don't have anything running this in the wild

Copy link
Author

Choose a reason for hiding this comment

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

I have commented out the extra db fields

@findsah findsah closed this Jan 9, 2024
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