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

bug-1928934: fix super search fields in signature report forms #6786

Merged
merged 2 commits into from
Nov 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ <h2 id="param-_results_offset">_results_offset</h2>

<p class="description">
Use this parameter to get a large number of results instead of setting
an arbitraty big <code>_results_number</code> value. Run queries in a
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'm pretty arbitraty sometimes.

an arbitrary big <code>_results_number</code> value. Run queries in a
loop, incrementing this by the value of <code>_results_number</code>,
and concatenate the content of the <code>hits</code> key.
</p>
Expand Down Expand Up @@ -146,7 +146,7 @@ <h2 id="param-_aggs.*">_aggs.*</h2>
run an aggregation on <code>field_1</code>, then for each bucket of
that aggregation, it will aggregate on <code>field_2</code>, and then
for each bucket of that sub-aggregation, it will aggregate on
<code>field_3</code>. Theoratically, we could have any number of
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Theoradically should be a word. It's not misspelled that way here, but it made me think a bit about it.

<code>field_3</code>. Theoretically, we could have any number of
levels of aggregations, but in practice we only allow all "level 1"
aggregations, and a few "level 2" and "level 3".
</p>
Expand Down Expand Up @@ -300,6 +300,9 @@ <h2 id="param-{{ filter.name }}">{{ filter.name }}</h2>
{% endfor %}
</p>

{# NOTE(willkg): we use permissions_needed so it matches the
protected crash schema rather than the webapp permissions which
users won't understand #}
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 added this note here so someone doesn't think this is a mistake. This will cause the data dictionary to show "public" and "protected" (processed crash schema permission terms) rather than "" and "crashstats.view_pii" (webapp-centric terms). The latter set of permissions are an internal implementation detail using terms we don't use in our documentation.

{% if filter.permissions_needed %}
<p class="permissions">{{ filter.permissions_needed | join(', ') }}</p>
{% endif %}
Expand Down
15 changes: 15 additions & 0 deletions webapp/crashstats/signature/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@

import pyquery

from django.contrib.auth.models import AnonymousUser
from django.urls import reverse
from django.utils.encoding import smart_str

from crashstats.crashstats import models
from crashstats.signature.views import get_fields
from socorro.lib.libdatetime import date_to_string, utc_now
from socorro.lib.libooid import create_new_ooid, date_from_ooid

Expand Down Expand Up @@ -661,3 +663,16 @@ def test_signature_bugzilla(self, client, db, es_helper):
< content.find("Related Crash Signatures")
< content.find("Bugs for <code>OOM | small</code>")
)

def test_get_fields(self, db, user_helper):
# Create anonymous user and get fields
user = AnonymousUser()
fields = get_fields(user)
len_public_fields = len(fields)
assert len(fields) > 0

# Create user with protected data access, get fields, and make sure there are
# more of them than if the user didn't have protected data access
user = user_helper.create_protected_user()
fields = get_fields(user)
assert len(fields) > len_public_fields
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Super search fields change over time, so I didn't want to create a test we had to update periodically. Instead, this asserts there are fields in the case where the user doesn't have protected data access and asserts that if the user does have protected data access, the user sees more fields than if the user didn't. That feels like a good enough set of assertions that aren't fragile.

27 changes: 19 additions & 8 deletions webapp/crashstats/signature/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,24 @@ def inner(request, *args, **kwargs):
return inner


def get_fields(user):
"""Retrieve super search fields this user has access to

:arg user: a Django User instance

:returns: a list of dicts with "id" and "text" keys

"""
return sorted(
x["name"]
for x in SuperSearchFields().get().values()
if x["is_exposed"]
and x["is_returned"]
and user.has_perms(x["webapp_permissions_needed"])
and x["name"] != "signature" # exclude the signature field
)


@track_view
@csp_update(CONNECT_SRC="analysis-output.telemetry.mozilla.org")
@pass_validated_params
Expand All @@ -84,14 +102,7 @@ def signature_report(request, params, default_context=None):

context["signature"] = signature

fields = sorted(
x["name"]
for x in SuperSearchFields().get().values()
if x["is_exposed"]
and x["is_returned"]
and request.user.has_perms(x["permissions_needed"])
and x["name"] != "signature" # exclude the signature field
)
fields = get_fields(request.user)
context["fields"] = [
{"id": field, "text": field.replace("_", " ")} for field in fields
]
Expand Down