-
Notifications
You must be signed in to change notification settings - Fork 223
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
Conversation
In 9b425e5, I changed the name of the field the webapp permissions were stored in, but missed updating the signature report view. This fixes that and adds a test.
# 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 |
There was a problem hiding this comment.
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.
@@ -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 #} |
There was a problem hiding this comment.
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.
@@ -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 |
There was a problem hiding this comment.
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.
@@ -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 |
There was a problem hiding this comment.
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.
This returns `fields` to being a list of field names so the rest of the code that depends on it works correctly.
^^^ @relud Does that look right to you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Thank you! |
In 9b425e5, I changed the name of the field the webapp permissions were stored in, but missed updating the signature report view. This fixes that and adds a test.
To test:
localhost:8000