#4593 Fix distribution export, item columns in alphabetically order #4617
Conversation
|
FYI The complaint from the business was that they were no longer in alpha order. That may be because the original items everyone starts out with happen to be in alpha order, but any they added would be showing at the end if they were in created order. |
|
ah I see, after seeing the header at first I also think it was already in alphabetical order unless seeing the last column, and after dig deeper I kinda confused bcs it was order by created_at, sorry for some inconvenience in my wording, thankyou. |
| @@ -118,7 +118,7 @@ def base_headers | |||
| def item_headers | |||
| return @item_headers if @item_headers | |||
|
|
|||
| @item_headers = @organization.items.order(:created_at).distinct.select([:created_at, :name]).map(&:name) | |||
| @item_headers = @organization.items.select("DISTINCT ON (LOWER(name)) items.name").order("LOWER(name) ASC").map(&:name) | |||
There was a problem hiding this comment.
I'm pretty sure there's a more rails-y way to do this. I haven't tried it out but it might be:
@organization.items.order('lower(name)').distinct.select([:created_at, :name]).map(&:name)
There was a problem hiding this comment.
I try by put binding.pry on it, but my terminal said this, pls enlighten me if this is bcs of my machine env
There was a problem hiding this comment.
Ok - so maybe I was wrong g . @awwaiid -- do you have a quick take on this?
There was a problem hiding this comment.
I asked a robot (Claude) and it suggested
@item_headers = @organization.items
.select(:name)
.group('LOWER(name)')
.order('LOWER(name) ASC')
.pluck('MIN(name)')
which isn't a direct translation but is actually sounds pretty good (it doesn't instantiate Item). mmm. OK, I asked it for something more direct and it gave
@item_headers = @organization.items
.select('DISTINCT ON (LOWER(items.name)) items.name')
.order('LOWER(items.name) ASC')
.pluck(:name)
which is back to basically what you have now.
I think since there isn't really a direct ActiveRecord thing for distinct on like this that what you have is fine, though you should consider the .pluck(:name) instead of the .map(&:name)
There was a problem hiding this comment.
After running a benchmark (screenshot), it turns out that map is actually a bit faster than pluck, though the difference isn't huge. Is it still better to stick with pluck, or should we consider using map? Both perform well. Let me know what you think!
|
@auliafaizahr: Your PR |
Checklist:
Resolves #4593
Description
Reorder item-related columns in the distribution export to be in lower-case alphabetical order.
Type of change
How Has This Been Tested?
Screenshots
before
after
as can we see in before image the last column was "Kit" after "Wipes" which "wipes" should be at last column if we order it alphabetically, which I try to fix to reorder it alphabetically and the result was in after images ("wipes" was in the last order)
At first I was confused because the title of the issue is "items columns are no longer in alphabetical order", but after dig deeper it was never in alphabetical order since first (found the discussion here #4070, it is said that it sorted by "created_at" column).
Look forward for feedback, thankyou.