Distinguish between given and display name for users

[emdiso]

made requested changes from: #1895

• removed requested files
• removed endpoint making a manual database entry
• removed edit display name modal
• changed the profile page table to a form, except for the CC users part which is simply shown below the form
• disabled all form fields except for display name
• added display_name field to staff user form as well
• added logic to profile_edit function in /evaluation/views.py to save changes

let me know if it all looks ok. very new to Django :)

keywords: first_name_given, first_name_chosen, choose name, custom first name.

[janno42]

Good work on the form! Again, I'm sorry this is so much work in your first PR but I'm glad you found a good solution!
There are a few more thing to address though. Please let us know if you need any further information.

[emdiso]

Hey, I think I addressed everything except for this test: In evap/evaluation/tests/test_views.py <#1903 (comment)>:

- self.assertIn(user.email, page)

+ # self.assertIn(user.email, page) # wont be in html anymore The email is still shown on the page (as it should be), so this needs to be reverted. It seems like the test is expecting the email to appear in the HTML template itself as text, which it doesn’t anymore since it is shown on the page dynamically. I am not too sure how to address this. Let me know if there is anything else/ if I hit everything necessary. Thanks!

…

On Mar 20, 2023, at 3:40 PM, Johannes Wolf ***@***.***> wrote: @janno42 requested changes on this pull request. Good work on the form! Again, I'm sorry this is so much work in your first PR but I'm glad you found a good solution! There are a few more thing to address though. Please let us know if you need any further information. In evap/evaluation/forms.py <#1903 (comment)>: > + self.fields["title"].initial = self.instance.title + self.fields["display_name"].initial = self.instance.display_name + self.fields["first_name"].initial = self.instance.first_name + self.fields["last_name"].initial = not self.instance.last_name + self.fields["email"].initial = not self.instance.email For each field except display_name please add self.fields["title"].disabled = "True" and similar In evap/evaluation/forms.py <#1903 (comment)>: > + title = forms.CharField(max_length=30, required=False, label=_("Title"), disabled=True) + display_name = forms.CharField(max_length=30, required=False, label=_("Display name")) + first_name = forms.CharField(max_length=30, required=False, label=_("First name"), disabled=True) + last_name = forms.CharField(max_length=30, required=False, label=_("Last name"), disabled=True) + email = forms.CharField(max_length=30, required=False, label=_("Email"), disabled=True) Django makes it easy to define forms by automatically creating fields based on the model's specification when using ModelForms like in this case. Here you are creating a ModelForm for a UserProfile. By defining which fields to use below (listing the fields in the order in which they should be shown in the form) Django knows that, e.g., the title should be a CharField. It is important to note that in the UserProfile model definition in evaluation/models.py we define max_length=255 for title and by letting Django handle the form creation, you don't have to redefine it and (unintentionally) redefine the length to 30 characters. So here you can remove all these lines. The only missing thing then is the disabled definition. This can be done at __init__ below, I added a comment there. In evap/evaluation/forms.py <#1903 (comment)>: > + self.fields["title"].initial = self.instance.title + self.fields["display_name"].initial = self.instance.display_name + self.fields["first_name"].initial = self.instance.first_name + self.fields["last_name"].initial = not self.instance.last_name + self.fields["email"].initial = not self.instance.email ⬇️ Suggested change - self.fields["title"].initial = self.instance.title - self.fields["display_name"].initial = self.instance.display_name - self.fields["first_name"].initial = self.instance.first_name - self.fields["last_name"].initial = not self.instance.last_name - self.fields["email"].initial = not self.instance.email These are not needed, Django will fill all fields with the object's values. In evap/evaluation/tests/test_views.py <#1903 (comment)>: > - self.assertIn(user.email, page) + # self.assertIn(user.email, page) # wont be in html anymore The email is still shown on the page (as it should be), so this needs to be reverted. On package-lock.json <#1903 (comment)>: This was a misunderstanding: Please do not delete the file and commit the deletion but instead remove this file from your commits. (Or to make things easier for you: Re-add the file as we will probably squash all your changes into one commit when merging.) On package.json <#1903 (comment)>: Same here On evap/contributor/tests/test_views.py <#1903 (comment)>: You can revert the changes in this file because the button doesn't exist anymore. On evap/evaluation/templates/profile.html <#1903 (comment)>: I wanted to suggest multiple changes to the UI and there was a bug with the two forms (when changing delegates, the display name would be deleted). This turned out to be quite a lot and while figuring out the UI, I had to build it anyway. So I put all the changes into a commit and pushed it to your branch. You're welcome to use it as-is. In evap/staff/forms.py <#1903 (comment)>: > @@ -1020,7 +1020,7 @@ def save(self, *args, **kw): ) # refresh results cache This is now also relevant for the profile page. Before, users couldn't change any cached data, but now we have to recache the results in case the display name of a user changed. Can you add this in the evaluation/forms.py as well? — Reply to this email directly, view it on GitHub <#1903 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYPCN4SD6L23TUEARZUAOUTW5CXCJANCNFSM6AAAAAAV4NGPCE>. You are receiving this because you authored the thread.

[janno42]

The test works fine for me. Actually, it sends a GET request (self.app.get) and then checks for text within the rendered page. Thus, the email field is filled and the user's email can be found on the page. So you can remove the comment and everything should be working :)

[janno42]

I didn't do a complete final review yet, but there should only be minor things, if any.

[janno42]

✔️ Meets requirements
✔️ UI functionality checked

[richardebeling]

Hey, thanks for working on this!

In an offline discussion, @janno42 and me found a few more spots that need to be changed that we forgot to list in the issue. I've added annotations for these starting with "(follow-up)". In order to not bloat the scope of this PR, we'd say its fine to ignore these for now, merge this pull request, and then create a follow up issue for these. However, if you'd already like to fix some of them here, you're also very welcome to do so.

In general, we much appreciate the work so far, and in case you don't have time to keep working on this, we're also happy to take over and implement the remaining changes ourselves. Just give us a small ping in this case, so we know we can go ahead.

[janno42]

✔️ Meets requirements
✔️ UI functionality checked

Fixed the issues

[niklasmohrin]

some comments (to be continued)

[niklasmohrin]

shouldn't this be automatic?

[richardebeling]

for some weird reason, it isn't -- but I haven't invested the time to find out why. Probably due to the complex query building above?

[niklasmohrin]

Looks mostly good, some nits. Haven't thought about whether there are more places yet

[niklasmohrin]

Looks good

[richardebeling]

Hey @emdiso, just pinging to let you know that we finally got to merge this, so your changes are now officially part of EvaP :) Thanks again for providing the initial code here 👍