[#137627] add links to 10 most recently purchased products to home page #1429

giladshanan · 2018-03-06T16:09:15Z

Release Notes

Add a list of the user's 10 most recently purchased products to the user's home page.

Screenshots

Context

Allow users to access frequently used products faster.

Refactorings

Rename sorted scope on Facilitiy model to alphabetized
Add MostRecentlyUsedSearcher service object for Products and Facilities
Refactor public_calendar view helper to handle non-specific facility context

meara

Hey Gilad--great start! Let me know if you have any questions about the comments I left you--we can also go through this together or pair a bit if that's helpful.

Also, can you add the redmine ticket number to the PR title? We usually do [#123456] pull request title as the format.

meara · 2018-03-06T16:55:14Z

config/locales/en.yml

  facility_downcase: facility
  facilities_downcase: facilities

+  Product: "!activerecord.models.product.one!"


I don't think we need these i18n fields for products. The reason we have them for facility is because Dartmouth uses "Resource" instead of "Facility" to refer to the facility model.

meara · 2018-03-06T16:55:37Z

config/locales/views/en.facilities.yml

    facilities:
      index:
        recently_used: Recently Used !Facilities!
+        recently_purchased: Recently Purchased !Products!


Per the ticket, I think this text should be "Recently Used Products"--you don't need to use the !Products! notation.

meara · 2018-03-06T17:10:26Z

app/models/product.rb

  scope :not_archived, -> { where(is_archived: false) }
  scope :mergeable_into_order, -> { not_archived.where(type: mergeable_types) }
  scope :in_active_facility, -> { joins(:facility).where(facilities: { is_active: true }) }
+  scope :recently_purchased, -> { joins(order_details: :order).order('ordered_at DESC').distinct.limit(10) }


Does .order(ordered_at: :desc) work here? If so, it would be better to use that format to take advantage of ActiveRecord.

That syntax results in a Unknown column 'products.orders' error, but I think I figured out how to do it with .merge

meara · 2018-03-06T17:13:19Z

app/views/facilities/index.html.haml

+
+- if @products.any?
+  %h4= text(".recently_purchased")
+  %table.table.table-striped.table-hover.product_list


I definitely get why you're using a table here; however, we don't typically use tables when listing products in this type of view, so it kind of breaks the style idiom for users, if that makes sense.

I would suggest modeling this after https://github.com/tablexi/nucore-open/blob/master/app/views/facilities/_product_list.html.haml#L7 -- that's what we use in facilities/show, so the link locations and style will match what the user expects.

meara · 2018-03-06T17:15:41Z

app/views/facilities/index.html.haml

+        %tr
+          %td
+            - if product.is_a?(Instrument)
+              = link_to product.name, [product.facility, product, :schedule]


So this link location, and the link on line 38, are actually linking to the admin views for the product. We want to link to the ordering views (see the product list partial I linked above), since this feature is for regular users trying to buy things, not admins wanting to manage products.

meara · 2018-03-06T17:22:34Z

Oh also, one more small note: I think we want the "Recently Used Products" to appear at the top of the page, above the list of all facilities.

giladshanan · 2018-03-09T23:26:54Z

@jhanggi can you give this a look-through when you have a chance? @meara and I both worked on it so it would be good to have another pair of eyes on it.

meara · 2018-03-09T23:45:43Z

app/models/product.rb

  scope :not_archived, -> { where(is_archived: false) }
  scope :mergeable_into_order, -> { not_archived.where(type: mergeable_types) }
  scope :in_active_facility, -> { joins(:facility).where(facilities: { is_active: true }) }
+  scope :recently_purchased, ->(user) { joins(order_details: :order).merge(Order.where(user: user, state: :purchased).group(:product_id).order('MAX(orders.ordered_at) DESC')).limit(10) }


We're not still using this, right?

meara · 2018-03-09T23:47:07Z

Also @giladshanan looks like you probably want to run rubocop -a on that new spec file.

jhanggi

Overall, great work. I like the separate object and the queries are definitely nicer now.

jhanggi · 2018-03-10T21:04:36Z

app/services/most_recently_used_searcher.rb

+  def recently_used_facilities(limit = 5)
+    return [] unless user
+
+    Facility.active.joins(products: { order_details: :order }).merge(Order.for_user(user).group("orders.facility_id").order('MAX(orders.ordered_at) DESC')).limit(limit)


Is there a good way to add some line breaks in here to make the line not so long and easier to read?

Also, use single quotes. And can you say group(orders: :facility_id)? (It’s very likely that doesn’t work, but would probably be cleaner)

So group(orders: :facility_id) doesn't work (the group method doesn't accept hash arguments) but group(:id) does the trick.

I struggle with understanding why group(:id) works. Might just be how the two scopes get merged. I think I would lean towards keeping the string in that case.

@meara it looks like Oracle doesn't like the new AR queries so we're gonna have to break it up into two steps like it was before.

jhanggi · 2018-03-10T21:05:24Z

app/views/facilities/index.html.haml


 .alert.alert-info= text(".welcome")

+- if @products.any?


I think it might be nicer to call this @recent_products.

jhanggi · 2018-03-10T21:07:49Z

spec/services/most_recently_used_searcher_spec.rb

+    end
+  end
+
+end


You use sublime, right? I would recommend adding the "ensure_newline_at_eof_on_save": true and ”trim_trailing_white_space_on_save": true settings.

Good call, thanks!

jhanggi · 2018-03-10T21:21:15Z

app/views/facilities/index.html.haml


+- if @products.any?
+  %h4= text(".recently_purchased")
+  .product_list{ class: @products.first.class.model_name.to_s.pluralize.downcase }


Is there a reason we can’t use the product_list partial itself?

Part of it may be related to sorting, and if that’s the case, we could pull the sorting out of that and do it in facilities/show, though to be honest, I think we should sort the products alphabetically. I think it’ll be easier to use that way.

If we do decide to decide to leave this here, there’s probably some cleanup we can do: I don’t think the classes here actually do anything.

It might also be nice to add the facility name. But I could go either way.

Also, as noted in the ticket, we want to show recent facilities first, then products.

The product_list partial expects a current_facility, which we don't have in this context. Is there an easy workaround that would let us use the partial anyway?

Sorting alphabetically seems fine to me. What do you mean about adding the facility name?

Add the facility name to the product link? Something like: Product name (Facility name)?

So facilities have a sorted scope and products have an alphabetized scope, they do very similar things. I lean towards alphabetized and sorted is only used in a handful of places. Do we want to change this or leave it alone?

…product links

giladshanan · 2018-03-12T15:44:20Z

Alphabetized + facility name

jhanggi

Very minor comments.

jhanggi · 2018-03-12T19:24:41Z

app/controllers/facilities_controller.rb

  # GET /facilities
  def index
    @facilities = Facility.active.sorted
+    @recently_used_facilities = MostRecentlyUsedSearcher.new(acting_user).recently_used_facilities.sorted


Should we create an aliased scope alphabetized for facilities? I do like that better than sorted. Probably want to leave sorted around for now because other stuff probably uses it. I'd be fine if we did that separately, though.

jhanggi · 2018-03-12T19:25:54Z

app/views/facilities/index.html.haml

+      %ul
+        - @recent_products.each do |product|
+          %li
+            = link_to "#{product.name}" + " - #{product.facility}", facility_product_path(product.facility, product)


Nitpicky, but I would do this all in one string: `"#{product.name} - #{product.facility}"

giladshanan · 2018-03-13T14:31:10Z

@meara @jhanggi It looks like .sorted was only used in a handful of places so I went ahead and added the change here. I also included a commit that makes a few adjustments to the product_list partial so that we can use it here. If we go that route we won't get the facility name in the list. I wasn't sure what your thoughts were on that so I figured I'd include it here so you can give me some feedback.

meara

This generally looks good, just a few small notes/questions.

Also, can you attach a screenshot of what it looks like post switch to product list partial?

meara · 2018-03-13T15:48:52Z

app/controllers/facilities_controller.rb

+    @facilities = Facility.active.alphabetized
+    @recently_used_facilities = MostRecentlyUsedSearcher.new(acting_user).recently_used_facilities.alphabetized
    @active_tab = "home"
+    @recent_products = MostRecentlyUsedSearcher.new(acting_user).recently_used_products.alphabetized


Are we sure that including the alphabetized scope here doesn't affect which products are selected as recently used? I'm a little leery of adding additional sorting in here because of the way active record chains scopes. Can we test this?

The alphabetized scope adds a secondary column to sort by, in this case the end of the query looks like this: ORDER BY MAX(orders.ordered_at) DESC, lower(products.name). Testing it out it doesn't seem to change the order at all. In fact, if you take a closer look at the first screenshot above, you'll see the products are not listed in alpha order. The ordering in the two more recent screenshots comes from the partial, which uses Array#sort on the products collection returned by @recent_products.

Should we ditch the alphabetized scope for products since it isn't doing much?

I was just about to suggest that 😀 . Leaving it in seems misleading, since the actual ordering is provided by the Array#sort in the partial.

It might be nice to pull the sort out of the partial and into the controller (and add a comment on why we're doing the sort in ruby and not in SQL)--you'd need to make sure you made the change for everywhere the partial is used, though.

Hm, it looks like this would mean putting 10 .sorts in the facilities/show view. Is there another way to clarify intent here? Maybe a code comment in the facilities controller where we call the MostRecentlyUsedSearcher's?

meara · 2018-03-13T15:50:44Z

app/views/facilities/_product_list.html.haml

      - products.sort.each do |product|
        %li{ class: product.class.model_name.to_s.downcase }
-          = public_calendar_link(product)
+          - if current_facility.present?


Does public_calendar_link require the facility? I think we can leave that in, I don't think having the calendar icon appear hurts anything, and it might be useful.

I agree, if we can include it here, I say we do it.

The public_calendar_link does require a facility, it links to facility_instrument_public_schedule_path. There's a way to make it work though. I'll put it in my next commit with the other changes so you can let me know what you think.

meara · 2018-03-13T15:51:57Z

app/views/facilities/_product_list.html.haml

+    - if current_facility.present?
+      %h3= product_list_title products, local_assigns[:title_extra]
+
    - if products.first.class.model_name == "TimedService"


I think the TimedService note ought to be nested under the if current_facility.present? as well. Try adding a Timed Service product type to see how it shows up.

meara · 2018-03-13T15:52:59Z

app/views/facilities/_product_list.html.haml

                = builder.text_field :quantity, value: 0, class: "product_quantity", index: nil
                = builder.hidden_field :product_id, value: product.id, index: nil
-          = link_to product.name + (product.is_hidden? ? ' (hidden)' : ''), facility_product_path(current_facility, product)
+          = link_to product.name + (product.is_hidden? ? ' (hidden)' : ''), facility_product_path(current_facility || product.facility, product)


I think this can just be product.facility, rather than using the or.

Also we should check for N+1s here.

jhanggi

Couple minor style notes, but otherwise I agree with all Meara's comments

jhanggi · 2018-03-13T18:23:56Z

app/services/most_recently_used_searcher.rb

+            .joins(products: { order_details: :order })
+            .merge(Order.for_user(user)
+                        .group(:id)
+                        .order('MAX(orders.ordered_at) DESC'))


Use double quotes for strings.

jhanggi · 2018-03-13T18:24:33Z

app/services/most_recently_used_searcher.rb

+           .joins(order_details: :order)
+           .merge(Order.for_user(user)
+                       .group(:id)
+                       .order('MAX(orders.ordered_at) DESC'))


Double quotes

jhanggi · 2018-03-13T18:25:12Z

app/views/facilities/_product_list.html.haml

      - products.sort.each do |product|
        %li{ class: product.class.model_name.to_s.downcase }
-          = public_calendar_link(product)
+          - if current_facility.present?


I agree, if we can include it here, I say we do it.

giladshanan · 2018-03-13T19:12:47Z

Partial view

giladshanan · 2018-03-13T19:14:30Z

Partial view with calendar (still working on the other suggested changes, but this is how it looks)

meara

Getting so close!

meara · 2018-03-13T21:55:25Z

app/helpers/products_helper.rb


  def public_calendar_options(product)
-    if current_facility.show_instrument_availability?
+    if current_facility&.show_instrument_availability?


I would switch this to current_facility&.show_instrument_availability? || !current_facility && product.facility.show_instrument_availability?, which is kind of gross, but ensures that we avoid N+1s in the facility context by using current_facility, and when we're in the home page context it will display with colors or not as appropriate (which is what show_instrument_availability? is for).

does current_facility&.show_instrument_availability? || product.facility.show_instrument_availability? do the trick?

The issue is we only want to call product.facility if current_facility is not present, rather than any time current_facility&.show_instrument_availability? evaluates to false. Does that make sense?

Yup, that does make sense, thanks.

meara · 2018-03-13T22:05:08Z

app/services/most_recently_used_searcher.rb

+  def recently_used_products(limit = 10)
+    return [] unless user
+
+    Product.active.in_active_facility


I would put includes(:facility) explicitly here, rather than in the in_active_facility scope.

meara · 2018-03-13T22:14:29Z

app/views/facilities/_product_list.html.haml

                = builder.text_field :quantity, value: 0, class: "product_quantity", index: nil
                = builder.hidden_field :product_id, value: product.id, index: nil
-          = link_to product.name + (product.is_hidden? ? ' (hidden)' : ''), facility_product_path(current_facility, product)
+          = link_to product.name + (product.is_hidden? ? ' (hidden)' : ''), facility_product_path(product.facility, product)


On second thought, I think what you had with current_facility || product.facility is better, because it lets us avoid all the includes we would need in the facility-specific context to prevent N+1s.

Specifically on this page: https://github.com/tablexi/nucore-open/blob/master/app/views/facilities/show.html.haml

jhanggi

Had some thoughts about how we're doing the sorting, but we can deal with that later.

jhanggi · 2018-03-15T23:36:36Z

app/controllers/facilities_controller.rb

+    @recently_used_facilities = MostRecentlyUsedSearcher.new(acting_user).recently_used_facilities.alphabetized
    @active_tab = "home"
+    # recent products get sorted alphabetically in the product_list partial
+    @recent_products = MostRecentlyUsedSearcher.new(acting_user).recently_used_products.includes(:facility)


Is there any downside to adding the alphabetization here, even if it's still re-sorted in the partial. We don't need to do it now, but I think it would be nice to not do the sort in the product_list partial (facilities/show is kind of nasty already, but could defiitely be cleaned up), but that can be for another time.

… controller

meara

👍 pending circle

giladshanan · 2018-03-16T19:13:32Z

🙌

…d in user (#1455) Release Notes Fix error on recently used products/facilities when there is no logged in user, follow up to #1429.

add links to 10 most recently purchased products to home page

cbc0998

giladshanan requested review from jhanggi and meara March 6, 2018 16:09

meara added the NU label Mar 6, 2018

meara suggested changes Mar 6, 2018

View reviewed changes

giladshanan changed the title ~~add links to 10 most recently purchased products to home page~~ [#137627] add links to 10 most recently purchased products to home page Mar 6, 2018

giladshanan added 5 commits March 9, 2018 09:38

filter products by user, user group instead of distinct

3cb1f46

refactor most recent facilities and products to service object

dd77bda

remove deprecated code

4b12cfe

rubocop

dae0ad7

add specs

88c741c

meara reviewed Mar 9, 2018

View reviewed changes

jhanggi reviewed Mar 10, 2018

View reviewed changes

refactor SQL queries, add alphabetical sorting, add facility name to …

7f8e93a

…product links

giladshanan added 2 commits March 12, 2018 10:53

rubocop

b8bf31d

rubocop again

f49cfa3

jhanggi approved these changes Mar 12, 2018

View reviewed changes

giladshanan added 2 commits March 13, 2018 09:06

replace sorted with alphabetized

abbfab8

use product list partial

5f2e7b5

meara reviewed Mar 13, 2018

View reviewed changes

jhanggi reviewed Mar 13, 2018

View reviewed changes

giladshanan added 2 commits March 13, 2018 16:35

use public calendar link, hide TimedService message, address N+1

8975fff

rubocop

7c5a101

meara reviewed Mar 13, 2018

View reviewed changes

giladshanan added 2 commits March 14, 2018 13:40

refactor to reduce N+1s

4101330

refactor queries to be Oracle friendly

a56f26d

meara approved these changes Mar 15, 2018

View reviewed changes

jhanggi approved these changes Mar 15, 2018

View reviewed changes

update conditional logic in public_calendar_options, sort products in…

42eb889

… controller

jhanggi approved these changes Mar 16, 2018

View reviewed changes

meara approved these changes Mar 16, 2018

View reviewed changes

giladshanan merged commit dc3e3a8 into master Mar 16, 2018

giladshanan deleted the 137627-last-ten-unhidden-products branch March 16, 2018 19:32

meara mentioned this pull request Mar 19, 2018

[#137627] recently used products/facilities still works when no logged in user #1455

Merged

meara added a commit that referenced this pull request Mar 20, 2018

[#137627] recently used products/facilities still works when no logge…

550c9dc

…d in user (#1455) Release Notes Fix error on recently used products/facilities when there is no logged in user, follow up to #1429.

+                  end
+                end
+              end

                
                    No newline at end of file

[#137627] add links to 10 most recently purchased products to home page #1429

[#137627] add links to 10 most recently purchased products to home page #1429

Uh oh!

Conversation

giladshanan commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release Notes

Screenshots

Context

Refactorings

Uh oh!

meara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meara commented Mar 6, 2018

Uh oh!

giladshanan commented Mar 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meara commented Mar 9, 2018

Uh oh!

jhanggi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giladshanan commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhanggi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giladshanan commented Mar 13, 2018

Uh oh!

meara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giladshanan Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giladshanan commented Mar 6, 2018 •

edited

Loading

giladshanan commented Mar 12, 2018 •

edited

Loading

giladshanan Mar 13, 2018 •

edited

Loading