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: Raise 404 when multiple person found in /community/personal/ #7038

Closed
wants to merge 1 commit into from

Conversation

kesara
Copy link
Member

@kesara kesara commented Feb 8, 2024

No description provided.

@kesara kesara force-pushed the fix/multiplepersonerror branch 2 times, most recently from 4cba512 to 051022d Compare February 9, 2024 01:22
Copy link

codecov bot commented Feb 9, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (4c396e6) 88.97% compared to head (051022d) 88.98%.
Report is 22 commits behind head on main.

❗ Current head 051022d differs from pull request most recent head ebc0f61. Consider uploading reports for the commit ebc0f61 to get more accurate results

Files Patch % Lines
ietf/community/views.py 25.00% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7038   +/-   ##
=======================================
  Coverage   88.97%   88.98%           
=======================================
  Files         291      291           
  Lines       40717    40717           
=======================================
+ Hits        36229    36233    +4     
+ Misses       4488     4484    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -54,7 +54,7 @@ def view_list(request, email_or_name=None):
try:
clist = lookup_community_list(request, email_or_name)
except MultiplePersonError as err:
return HttpResponse(str(err), status=300)
raise Http404(str(err))
Copy link
Member

Choose a reason for hiding this comment

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

We should either 404 with no decoration or use some other response. I'm not quickly coming up with a use-case for telling someone that the string entered matches multiple people is helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah! 404 page doesn't show any decorations. I'll remove error strings.

Copy link
Member

@jennifer-richards jennifer-richards left a comment

Choose a reason for hiding this comment

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

Would it make sense to push the MultiplePersonError -> 404 logic down into lookup_community_list()? Is it used anywhere that a 404 isn't the right thing to do?

(I'm not actually a fan of helper directly 404ing, but that's a bigger set of changes...)

@rjsparks
Copy link
Member

Would it make sense to push the MultiplePersonError -> 404 logic down into lookup_community_list()? Is it used anywhere that a 404 isn't the right thing to do?

(I'm not actually a fan of helper directly 404ing, but that's a bigger set of changes...)

Yes, I think this would make sense, and it would obviate the need for the MultiplePerssonError exception in the first place - the utility could simply raise the 404 instead.

@rjsparks
Copy link
Member

rjsparks commented Sep 4, 2024

@kesara @jennifer-richards : Is this still something we wish to pursue?

@jennifer-richards
Copy link
Member

jennifer-richards commented Sep 4, 2024

@kesara @jennifer-richards : Is this still something we wish to pursue?

I think so, though I don't recall in detail the circumstances leading to it. I guess it's replacing 500s with 404s?

[edit] or I guess 300s with 404s, based on the test changes

@kesara
Copy link
Member Author

kesara commented Sep 10, 2024

@kesara @jennifer-richards : Is this still something we wish to pursue?

I think so, though I don't recall in detail the circumstances leading to it. I guess it's replacing 500s with 404s?

[edit] or I guess 300s with 404s, based on the test changes

@jennifer-richards, This was discussed on Slack.
Then, @rjsparks suggested returning 404 such cases.

@kesara
Copy link
Member Author

kesara commented Nov 3, 2024

OBE

@kesara kesara closed this Nov 3, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants