search

apluslms / a-plus

A+ frontend portal - A+ LMS documentation:
https://apluslms.github.io/
Other
67 stars 73 forks source link

Improve the usability of the admin interface #707

Closed markkuriekkinen closed 3 years ago

markkuriekkinen commented 4 years ago

A+ admins waste time when they need to use the Django admin interface, but it is slow to find the necessary data. Some admin pages load very slowly or the search filters are not shown in the user interface. It is possible to filter the list view by adding GET query parameters to the URL, but it would be easier to access filters in the user interface.

jaguarfi commented 3 years ago

* [ ] The participant's view needs a search functionality to filter the list of participants

markkuriekkinen commented 3 years ago 
* [ ]  The participants view needs a search functionality to filter the list of participants

@jaguarfi are you referring to the participants page in the course or some Django admin view (URL /admin)? This issue is about the Django admin pages and the course pages should be covered in another issue.

jaguarfi commented 3 years ago

Yes, you are right. This is not the place for that issue. I created a new issue for that specific feature https://github.com/apluslms/a-plus/issues/770

skulonen commented 3 years ago

@Mankro Some questions:

  1. I've replaced all user dropdowns with simple text fields using the raw_id_fields option. The fields do not accept usernames or student IDs -- only the internal IDs used in the database. There is a performant search popup option, though. Is this okay? image
  2. All detail pages in the admin UI now open quickly because of the raw_id_fields option, except for the Token one: image That one is defined in the 3rd party library rest_framework instead of in A+ code. If we want, we can also improve that page's performace by manipulating the TokenAdmin class like this: from rest_framework.authtoken.admin import TokenAdmin TokenAdmin.raw_id_fields = ["user"] But maybe that is a bit of a hack? And I'm not sure if there is a good, obvious place to put the above snippet (settings.py???).
  3. "Add search to the admin list views": is there a list of fields that should be searchable? Or should I just choose some fields for each model using my own judgement?
  4. Regarding the last two checkboxes: the fields are not shown in the admin UI because the auto_now_add=True option makes the field read only, and the admin UI only displays editable fields by default. We can force the admin UI to display the field in a noneditable format using ModelAdmin.readonly_fields, or we can make the field editable using this trick. Which one should we do?
  5. Style question: some of the admin classes use single quotes and some use double quotes. Some use lists and some use tuples, some use a bit of both. When editing these classes, should I a) use the style that the class already uses, b) use the style defined in our style guidelines, c) convert all of these classes to conform to the style guidelines?
skulonen commented 3 years ago

Actually, regarding point 2, it seems that the REST framework guide specifically recommends the "hack":

In case you are using a large user base, we recommend that you monkey patch the TokenAdmin class to customize it to your needs, more specifically by declaring the user field as raw_field.

https://www.django-rest-framework.org/api-guide/authentication/

markkuriekkinen commented 3 years ago
  1. yes, good!
  2. yes, let's use the monkey patch from the DRF docs. They say that the code should be placed in the file your_app/admin.py. So is it aplus/admin.py or userprofile/admin.py?
  3. you can start by selecting the search fields with your own judgement. I would be interested in searching by the user ("all submissions from this user") and exercise ("all submissions to this exercise"). Note that for users, the student id is stored in the other table userprofile.
  4. I want to see current database values in the Django admin. Read only is good.
  5. Let's start using new, consistent style in the admin classes. Could you make one git commit for fixing styles without changing the functionality and change the functionality in another commit? Do our style guides define the correct style for this case or shall we define the style now? If our style guide is inadequate, we could follow the style used in the Django admin docs (examples).
skulonen commented 3 years ago

@Mankro

They say that the code should be placed in the file your_app/admin.py. So is it aplus/admin.py or userprofile/admin.py?

Well, A+ consists of multiple apps each with their own admin module. aplus/admin.py does not exist, and is never executed if created because "aplus" is not an app. So we'll have to put the snippet in one app's admin module. I guess it's most closely related to user profiles, so I put it in userprofile/admin.py.

Do our style guides define the correct style for this case or shall we define the style now?

According to our style guide, we should use single quotes for strings not shown to the user and have one item per line in lists. Which means lines like this search_fields = ["label", "title"] should be like this (I know the indentation is wrong) search_fields = [ 'label', 'title' ]

Regarding lists vs tuples when defining lists of fields: the Django documentation seems to use a mix of both. Because of this, I propose we use tuples because a) they're not supposed to be mutated, b) Django source code initializes them as empty tuples (search_fields = ()), c) tuples are apparently slightly more efficient.

markkuriekkinen commented 3 years ago

@skulonen Sounds good, thanks! Let's continue like you said. Yes for tuples.

You can write multiline code with three backticks:

search_fields = [
    'label',
    'title'
]
skulonen commented 3 years ago

@Mankro One more question regarding this task:

Usually when a user is displayed in menu like a dropdown, it would be helpful to see the user's email in addition to the name, username and student ID.

We can achieve this by changing the UserProfile model's __str__ method to include the email, like this: {username} ({first_name} {last_name}, {email}, {student_id}). However, this would also affect the end-user UI of A+, wherever the __str__ value is used. At least in the deviaton template: <td>{{ deviation.submitter }}</td>.

Is it okay to display the email in the end-user UI or should I come up with another solution (adding email to UserProfileAdmin.list_display)?

markkuriekkinen commented 3 years ago

@Mankro One more question regarding this task:

Usually when a user is displayed in menu like a dropdown, it would be helpful to see the user's email in addition to the name, username and student ID.

We can achieve this by changing the UserProfile model's __str__ method to include the email, like this: {username} ({first_name} {last_name}, {email}, {student_id}). However, this would also affect the end-user UI of A+, wherever the __str__ value is used. At least in the deviaton template: <td>{{ deviation.submitter }}</td>.

Is it okay to display the email in the end-user UI or should I come up with another solution (adding email to UserProfileAdmin.list_display)?

Yes, let's change the __str__ method. Since it already includes the username, which is not supposed to be shown to everyone such as fellow students, there can't be any harm with adding the email too. Teachers are allowed to see the student's email. If the teacher can see the student's username, well, they really shouldn't, but that can be fixed later. It is not important this spring.

You can now use f"strings {var}" instead of the old .format() method. We are now always running Python 3.6+ (in practice 3.8 in Ubuntu 20.04 and 3.7 in Debian Buster based development containers) https://github.com/apluslms/a-plus/blob/80ce6aea9582b3d4ae54c8f6e33a360c71cd40f4/userprofile/models.py#L44-L48