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

[#2261, #2262, #2263] Fix display name in profile #1342

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

pi-sigma
Copy link
Contributor

@pi-sigma pi-sigma commented Aug 9, 2024

  • Use display name ("roepnaam") in profile welcome message by default
  • Use first name as fallback in case roepnaam is not set
  • Show roepnaam in profile overview

Taiga: #2261, #2262, #2263

    - use display name ("roepnaam") by default
    - use first name as fallback in case display name is not set
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.10%. Comparing base (69630e7) to head (6ef9f62).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1342   +/-   ##
========================================
  Coverage    95.09%   95.10%           
========================================
  Files          994      994           
  Lines        36244    36248    +4     
========================================
+ Hits         34468    34472    +4     
  Misses        1776     1776           

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

@pi-sigma pi-sigma marked this pull request as ready for review August 12, 2024 09:49
@pi-sigma pi-sigma requested a review from swrichards August 12, 2024 09:49
@@ -354,6 +354,10 @@ def get_full_name(self):
def get_short_name(self):
return self.first_name

@property
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is mildly confusing because display_name is used elsewhere (e.g. in the profile edit page) to signify the Dutch word "roepnaam", which this would replicate. Perhaps addressable_name to indicate its purpose in addressing the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you find confusing about call_name? It doesn't replicate display_name because it returns either the display_name (if it's set) or the first_name. Am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I found it confusing that they refer to the same concept, e.g. the code here, which suggests that we treat display_name as equal to "Roep Naam".

Differently put, I think our names should actually be reversed:

  • The model field display_name should be call_name
  • The property here should be display_name

But: given that this is slated for deprecation, let's not worry about it and move forward with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I get it. I stumbled over this myself and couldn't come up with a great name, since "display_name" would already be appropriate.

@swrichards swrichards requested a review from alextreme August 14, 2024 12:19
@alextreme alextreme merged commit 82a1eb6 into develop Aug 14, 2024
18 checks passed
@alextreme alextreme deleted the issue/2662-profile-welcome branch August 14, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants