refactor: Rework community views to operate on Person instead of User#5899
Conversation
…he URL This only happens using the form-filling and submission feature of WebTest, which is only used in this one test case, so just it rip out.
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
I have a few suggestions. The only one I consider essential is giving a clear error description if lookup_community_list encounters multiple matches.
| if hasattr(request.user, 'person') and request.user.person in persons: | ||
| person = request.user.person | ||
| else: | ||
| raise MultiplePersonError("\r\n".join([p.user.username for p in persons])) |
There was a problem hiding this comment.
I think this will just return a list of usernames, which winds up displayed as the response if a user hits this. It'd be nice to provide a bit more context in the error message.
(If you're testing this in the dev browser, that often provides a much richer response when it hits an exception than happens in production)
There was a problem hiding this comment.
Yeah, I'm not really happy with it, but it was copied from Henrik's code in person.views.photo. I played with the idea of returning a selection list, or of showing all matching community lists on one page (as in person.views.profile), but ultimately punted in favor of preferring the logged-in user - if I'm John Doe, I probably want my community list rather than any other John Doe's. Ultimately, this is (hopefully) an extreme edge case.
There was a problem hiding this comment.
That makes sense - I'm just imagining something like "Found multiple matches: " at the start of the error message to give the response some meaning for a poor user who happens upon this.
|
I'm planning to hold this for merge/release until after IETF 117 |
|
I was being silly - the plan is to hold merging feat/user2person into main - no reason to block merging this PR into feat/user2person. |
3 participants
Fixes #5859