flake8
-
reviewboard/webapi/resources/search.py (Diff revision 1)
Review Request #9924 — Created May 10, 2018 and submitted
Previously we were indexing the full name, which allowed users to search
by full name. However, if a user has marked their profile as private, we
do not want them to be searchable by their name.Additionally, this patch addresses two further issues:
The
show_profile
attribute was stored as aBooleanField
, but
there is a longstanding Haystack bug that results in boolean fields
always being returned from Whoosh asTrue
. This has been replaced with
a custom BooleanField with the correct behaviour.The
show_profile
as indexing the result ofUser.is_profile_visible()
,
which will always returnFalse
when there is no user argument
supplied. Instead, we now store theis_private
attribute of
Profile
directly on the index.Finally, the search API has been updated to strip the full name out of
results when using the database query backend (i.e., when either search
or on-the-fly indexing is disabled).
Ran unit tests.
Description | From | Last Updated |
---|---|---|
E303 too many blank lines (2) |
reviewbot | |
E303 too many blank lines (2) |
reviewbot | |
E303 too many blank lines (2) |
reviewbot | |
E303 too many blank lines (2) |
reviewbot | |
This should be stripped out for private profiles as well. |
chipx86 | |
This function returns a single unicode string, not a list. |
chipx86 | |
Backticks in summaries can render strangely. We should just show the text as-is. |
chipx86 | |
Missing "Returns". |
chipx86 | |
We don't want to use get_profile() in search indexes, because any users without a profile will end up triggering database … |
chipx86 | |
Same as above regarding backticks. |
chipx86 | |
This is missing a description. |
chipx86 | |
Same comment about backticks. |
chipx86 | |
Same regarding get_profile usage. |
chipx86 | |
Blank line between these. |
chipx86 | |
What license is that under? |
chipx86 | |
This bit would be faster as: try: return self.value_map[value] except KeyError: return bool(value) |
chipx86 | |
How about: if isinstance(instance, Profile): kwargs['sender'] = User instance = instance.user self.handle_save(...) |
chipx86 | |
I don't believe filters are allowed in a blocktrans. This may need more closer testing. Instead, I think you can … |
chipx86 | |
The e-mail address also needs to be wrapped in this. |
chipx86 | |
Can you print the resulting expression and make sure the grouping for the full name query ends up looking like … |
chipx86 | |
Can this be moved into the initial query = above? |
chipx86 | |
Missing the unicode_literals import. |
chipx86 |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+181 -12) |
Add search/fields.py
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+218 -12) |
Fix actual indexed text
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+187 -7) |
Fix actual indexed text
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+330 -14) |
Post the correct change
Rebasing. Replace more usages.
Depends On: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 7 (+353 -21)
|
reviewboard/accounts/search_indexes.py (Diff revision 7) |
---|
This should be stripped out for private profiles as well.
reviewboard/accounts/search_indexes.py (Diff revision 7) |
---|
This function returns a single unicode string, not a list.
reviewboard/accounts/search_indexes.py (Diff revision 7) |
---|
Backticks in summaries can render strangely. We should just show the text as-is.
reviewboard/accounts/search_indexes.py (Diff revision 7) |
---|
We don't want to use
get_profile()
in search indexes, because any users without a profile will end up triggering database changes. This will be slow and disruptive. Instead, we need to see if a profile was fetched for the user at all (query cache has a value) and operate based on that.We should probably do that in a private method in this class, or maybe add a
cached_only=
argument toget_profile()
?
reviewboard/search/fields.py (Diff revision 7) |
---|
This bit would be faster as:
try: return self.value_map[value] except KeyError: return bool(value)
reviewboard/search/signal_processor.py (Diff revision 7) |
---|
How about:
if isinstance(instance, Profile): kwargs['sender'] = User instance = instance.user self.handle_save(...)
reviewboard/templates/search/_review_request.html (Diff revision 7) |
---|
I don't believe filters are allowed in a
blocktrans
. This may need more closer testing.Instead, I think you can do:
and result.author|default:result.username as author
.
reviewboard/templates/search/indexes/auth/user_text.txt (Diff revision 7) |
---|
The e-mail address also needs to be wrapped in this.
reviewboard/webapi/resources/search.py (Diff revision 7) |
---|
Can you print the resulting expression and make sure the grouping for the full name query ends up looking like
((a | b) & is_private=True)
? I want to be sure the grouping is going to be future-proof.
reviewboard/webapi/resources/search.py (Diff revision 7) |
---|
Can this be moved into the initial
query =
above?
Addressed issues.
Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+444 -23)
|
Remove unused import.
Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+444 -24)
|
Addressed all remaining feedback.
Commit: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+441 -24)
|
Fix unit tests that failed as a result of changes to /r/9930
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+445 -26) |