-
Notifications
You must be signed in to change notification settings - Fork 378
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: Use email or name when building community list view #7203
fix: Use email or name when building community list view #7203
Conversation
instead of email
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7203 +/- ##
==========================================
+ Coverage 88.78% 88.83% +0.04%
==========================================
Files 296 304 +8
Lines 41320 41501 +181
==========================================
+ Hits 36687 36866 +179
- Misses 4633 4635 +2 ☔ View full report in Codecov by Sentry. |
Since this is fixing a crash bug, we should add, or modify an existing test to include, a test that exercises the condition that led to the crash. That test should also cover the view for person Y after having constructed a person X with name N and email E, and a person Y with name N (note same name) but no email. |
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.
See the earlier note re: tests
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.
Looks good to me, agree that it should have a test.
I added a test - which fails because it turns out this goes deeper. There's a similar issue in |
Thanks for having a go at it! (It was on my list at the code sprint, but I ran out of time.) I'll have a look at the files you mentioned when I can. |
@jennifer-richards @microamp Is this still something to pursue? |
Yes, original bug is still there afaik and I think this was a good approach. |
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
In my previous comments I was worried because the email_or_name
variable is not set when the list_menu.html
is included via group_documents.html
in the group_documents()
view. On closer review, there are guards in place so the email_or_name
is not needed in the group context.
Fixes #7036
An additional argument has been added to pass the
email_or_name
variable through.