Skip to content

Feature change ordering in employees' list#557

Merged
kbeker merged 1 commit intomasterfrom
feature-change-employees-list-ordering
May 24, 2020
Merged

Feature change ordering in employees' list#557
kbeker merged 1 commit intomasterfrom
feature-change-employees-list-ordering

Conversation

@maciejSamerdak
Copy link
Collaborator

Changes ordering in employees list in following hierarchy:
user type -> last name -> first name -> email

@maciejSamerdak maciejSamerdak added the feature New feature label May 16, 2020
@maciejSamerdak maciejSamerdak added this to the next_release milestone May 16, 2020
@maciejSamerdak maciejSamerdak requested a review from kbeker May 16, 2020 15:37
@maciejSamerdak maciejSamerdak self-assigned this May 16, 2020
@maciejSamerdak maciejSamerdak force-pushed the feature-change-employees-list-ordering branch from 8d8be88 to 9ddf4c8 Compare May 16, 2020 16:16
Copy link
Contributor

@kbeker kbeker left a comment

Choose a reason for hiding this comment

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

There is no test at all. Please add

users/views.py Outdated
Comment on lines +157 to +160
admins = self.queryset.filter(user_type=CustomUser.UserType.ADMIN.name)
managers = self.queryset.filter(user_type=CustomUser.UserType.MANAGER.name)
employees = self.queryset.filter(user_type=CustomUser.UserType.EMPLOYEE.name)
context_data["ordered_object_list"] = list(admins) + list(managers) + list(employees)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract this to private methods for example something like this:

def _get_users_by_user_type(self, user_type):
    return self.queryset.filter(user_type=user_type)

def _get_ordered_users_by_user_type(self):
    admins = self._get_users_by_user_type(user_type=CustomUser.UserType.ADMIN.name)
    managers = self._get_users_by_user_type(user_type=CustomUser.UserType.MANAGER.name)
    employees = self._get_users_by_user_type(user_type=CustomUser.UserType.EMPLOYEE.name)
    return list(admins) + list(managers) + list(employees)

context_data["ordered_object_list"] = self._get_ordered_users_by_user_type()

Also there is no test for this - please add some.

I can't find different way to order all users in one query, but I can't believe that there is not other way to query that queryset should by ordered by uset_type where user_type first is ADMIN then MANAGER etc. But don't waste time for this right now. I just telling you this to keep it in mind :)

Copy link
Collaborator Author

@maciejSamerdak maciejSamerdak May 19, 2020

Choose a reason for hiding this comment

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

Actually, I could try to either use Python's sorted (but this one returns a list) or write a function in the model manager. I was hesitant to use the latter, because I found it too complex for such simple task, but now that I look at it again, I think I could try it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, I checked that model manager solution and it involves calling annotate to create a new integer field. It's values would be set based on the value of the field we want to sort by (so for "ADMIN" we set 0, for "MANAGER" we set 1, etc.). Then we call .order_by and pass that new field we created.
Seems like it's impossible in Django to fiddle with database ordering on the backend side.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we could accomplished that with some changes in Employee model. I just though about changing user type into IntEnums and by this we could get that really simple sorting mechanism but I think that we for this moment we can leave as it is:)

@maciejSamerdak maciejSamerdak force-pushed the feature-change-employees-list-ordering branch from 2412f32 to e632d2f Compare May 24, 2020 18:57
@kbeker kbeker force-pushed the feature-change-employees-list-ordering branch from e632d2f to c09ddf7 Compare May 24, 2020 20:36
@kbeker kbeker merged commit 3921fc6 into master May 24, 2020
@kbeker kbeker changed the title Change ordering in employees' list Feature change ordering in employees' list May 24, 2020
@kbeker kbeker deleted the feature-change-employees-list-ordering branch December 30, 2020 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants