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

fix engagements filters in 'engagements by product view' #9913

Closed
wants to merge 0 commits into from

Conversation

davidhernandeze
Copy link
Contributor

sc-[5299]

Description

Engagements filters inside the "Engagements By Product" view are not working properly. Searching by the exact name in the "Engagement name" filter is returning incorrect results.

The Problem

Building relationship filters as we currently do in EngagementFilter generates a query that gets filtered products with all the related engagements, where at least one engagement satisfies the filter criteria. The engagements_all view iterates over the engagements of each product to render the table rows. As the result, we ended up showing all engagements of the products that satisfied the filter criteria. For example, if there are two engagements that satisfied the filter criteria from two different products, the rendered table will show all engagements that belong to those two products.

Solution

Using an additional filter when prefetching the engament_set will filter the engagements at the query level, so only filtered engagements related to filtered products will be obtained.

engagement_query = Engagement.objects.annotate(test_count=Count('test__id'))
filter_qs = products_with_engagements.prefetch_related(
   Prefetch('engagement_set', queryset=ProductEngagementsFilter(request.GET, engagement_query).qs)
)

Also, the paginator object had to be modified, to represent the correct amount of rows that are currently shown in the page.

prods.paginator.count = sum(len(prod.engagement_set.all()) for prod in prods)

Copy link

dryrunsecurity bot commented Apr 10, 2024

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security Status Findings
Configured Codepaths Analyzer 0 findings
Authn/Authz Analyzer 0 findings
AppSec Analyzer 0 findings
Sensitive Files Analyzer 0 findings
Secrets Analyzer 0 findings

Note

🟢 Risk threshold not exceeded.

Powered by DryRun Security

@davidhernandeze
Copy link
Contributor Author

sc-[5299]

@mtesauro
Copy link
Contributor

@davidhernandeze You don't have to fix the Ruff linter errors, those were fixed in #9903 and outside the changes you've made here.

Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

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

Approved

@davidhernandeze
Copy link
Contributor Author

I'm sorry. I made a mistake using my dev branch, which was causing me problems opening more pull requests using my fork. I will duplicate this PR with the correct branching strategy.

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.

3 participants