Marketplace Tags enhancements #2431

rosschapman · 2024-06-01T00:44:54Z

A handful of useful, low-bar follow-ups to #2361 that was scoped out in #2426.

Adds tag deletion
Renames group* -> menu*
💬 Did my best to migrate this language in the code and for end users. Feedback very welcome.
Adds sort_alpha scope for Products
Adds new Display and Admin components for tags
- Adds product tag count to Tag admin, with pluralized string

rosschapman · 2024-06-02T03:31:31Z

app/furniture/marketplace/locales/en.yml

@@ -70,6 +70,7 @@ en:
        success: "Notification Method '%{contact_location}' Saved!"
      destroy:
        success: "Notification Method '%{contact_location}' Removed!"
+        link_to: "Remove Notification Method '%{contact_location}'"


Discovered hitting a different bug when running testing spec/furniture/marketplace/marketplace_tags_system_spec.rb surfaced this missing translation. 🤷‍♀️

Probably worth submitting independently so it can be merged without being hung up on feedback for this feature.

rosschapman · 2024-06-03T17:55:58Z

app/components/marketplace/tag/admin_component/admin_component.html.erb

+<%= render CardComponent.new(dom_id: dom_id(tag)) do |card| %>
+  <div class="flex flex-col gap-2">
+    <h3><%= tag.label %></h3>
+    <span class="text-xs">Assigned to <%= assigned_product_count %> <%= "Product".pluralize(assigned_product_count) %></span>


Not sure about "assigned to" 🤔

I think just "N products" or "on N products" might be clear enough.

rosschapman · 2024-06-03T17:56:52Z

app/components/marketplace/tag/admin_component/admin_component.yml

Do we care about this boilerplate being here?

Is this for i18n? Or ? If we don't put anything in it I would rather we left it out.

Yeah it's auto-generated by rails generate component ... for i18n. I can remove them.

Yeah, no sense in checking in blank lines. (Also, I had no idea this .yml file for a component was a thing, #TIL.)

rosschapman · 2024-06-03T17:58:08Z

app/furniture/marketplace/menu_component.html.erb

  <%- unless tag.products.empty? %>
    <h1><%= tag.label %></h1>
    <div class="grid lg:grid-cols-3 gap-3">
-      <%- tag.products.with_all_rich_text.unarchived.each do |product| %>
+      <%- tag.products.with_all_rich_text.unarchived.sort_alpha.each do |product| %>


Is this amount of scope chaining typical in templates? Would you put a helper on the ruby component to reduce logic in the template?

Well, it's not too unusual, but it does make for an N+1 query. It may be worth pulling this into the query on line 1; which may be expressed in the view directly, as a helper on MenuComponent, or a scope on Marketplace.

There's also an argument to be made for not doing any queries from within a Component or view; instead performing them at the controller layer.

What's the best way to analyze whether this lookup would produce an n+1 query? Righ now if I render out the sql in a rails console I'm not seeing multiple selects for associative rows.

select "marketplace_products"."id" as t0_r0, "marketplace_products"."marketplace_id" as t0_r1, "marketplace_products"."name" as t0_r2, "marketplace_products"."price_cents" as t0_r3, "marketplace_products"."price_currency" as t0_r4, "marketplace_products"."created_at" as t0_r5, "marketplace_products"."updated_at" as t0_r6, "marketplace_products"."discarded_at" as t0_r7, "marketplace_products"."servings" as t0_r8, "action_text_rich_texts"."id" as t1_r0, "action_text_rich_texts"."name" as t1_r1, "action_text_rich_texts"."body" as t1_r2, "action_text_rich_texts"."record_type" as t1_r3, "action_text_rich_texts"."record_id" as t1_r4, "action_text_rich_texts"."created_at" as t1_r5, "action_text_rich_texts"."updated_at" as t1_r6 from "marketplace_products" inner join "marketplace_product_tags" on "marketplace_products"."id" = "marketplace_product_tags"."product_id" left outer join "action_text_rich_texts" on "action_text_rich_texts"."record_type" = 'Marketplace::Product' and "action_text_rich_texts"."name" = 'description' and "action_text_rich_texts"."record_id" = "marketplace_products"."id" where "marketplace_product_tags"."tag_id" = 'b8d56ff4-8326-4a9b-8576-9e4b677bc4c4' and "marketplace_products"."discarded_at" is null /* loading for pp */ order by "marketplace_products"."name" asc limit 11;

An N+1 situation requires two separate queries (one that is repeated N times, plus the outer one), so you would not see an N+1 query by looking at the SQL of one query. You would need to have a look at the implementation of the ProductComponent, which is what you are rendering inside the loop, and see if it is going to create any additional queries.

Also, we can add https://github.com/flyerhzm/bullet to the project and help it guide us when it thinks there are N+1 queries (although note that it is not, err, bullet-proof?).

Oh oh oh right, the risk of n+1 occurs with potential queries for each item from this collection as we loop. bullet gem looks useful. Sounds like similar amount of caution/skepticism we reserve for strong_migrations's suggestions 😅.

I'll take a pass in another PR adding the bullet gem and lifting this heavy lifting to a better place for Law of Demeter and n+1 code hazards.

rosschapman · 2024-06-03T18:01:03Z

app/furniture/marketplace/product/title_component.html.erb

-<h3><%= name %></h3>
+<div class="mb-2">
+  <h3><%= name %></h3>
+</div>


I've removed some markup where it didn't seem to be doing what it was meant to do. In this case the margin wasn't working as a result of how parent flex/grid containers are set up.

zspencer

This looks like an overall improvement from an ixd perspective, and tidies up some oddities; I'm down.

zspencer · 2024-06-03T18:07:21Z

app/components/marketplace/tag/admin_component/admin_component.yml

Is this for i18n? Or ? If we don't put anything in it I would rather we left it out.

zspencer · 2024-06-03T18:08:04Z

app/furniture/marketplace/locales/en.yml

@@ -70,6 +70,7 @@ en:
        success: "Notification Method '%{contact_location}' Saved!"
      destroy:
        success: "Notification Method '%{contact_location}' Removed!"
+        link_to: "Remove Notification Method '%{contact_location}'"


Probably worth submitting independently so it can be merged without being hung up on feedback for this feature.

zspencer · 2024-06-03T18:08:18Z

app/furniture/marketplace/locales/en.yml

@@ -142,6 +143,8 @@ en:
      new:
        link_to: "Add Product Tag"
      edit:
-        link_to: "Edit Product Tag"
+        link_to: "Edit Product Tag '%{label}'"


👍👍👍

zspencer · 2024-06-03T18:08:20Z

app/furniture/marketplace/locales/en.yml

      update:
        success: "Product Tag saved!"
+      destroy:
+        link_to: "Remove Product Tag '%{label}'"


👍👍👍

zspencer · 2024-06-03T18:11:35Z

app/furniture/marketplace/menu_component.html.erb

  <%- unless tag.products.empty? %>
    <h1><%= tag.label %></h1>
    <div class="grid lg:grid-cols-3 gap-3">
-      <%- tag.products.with_all_rich_text.unarchived.each do |product| %>
+      <%- tag.products.with_all_rich_text.unarchived.sort_alpha.each do |product| %>


Well, it's not too unusual, but it does make for an N+1 query. It may be worth pulling this into the query on line 1; which may be expressed in the view directly, as a helper on MenuComponent, or a scope on Marketplace.

There's also an argument to be made for not doing any queries from within a Component or view; instead performing them at the controller layer.

zspencer · 2024-06-03T18:13:57Z

app/furniture/marketplace/tags_controller.rb

+        format.turbo_stream do
+          if mtag.destroyed?
+            render turbo_stream: turbo_stream.remove(mtag)
+          else
+            render turbo_stream: turbo_stream.replace(mtag)
+          end
+        end


Do we need a turbo stream? I thought we had gotten to the point where we can just render plain html and Turbo will do a diff-and-swap within the marketplace?

Ah right. I'll double check.

anaulin · 2024-06-03T20:09:48Z

app/components/marketplace/tag/display_component/display_component.yml

Can be deleted.

rosschapman · 2024-06-03T21:34:01Z

@anaulin I'm going to merge for the dopamine hit but if you're still chewing on anything please leave it here and I'll address it later.

rosschapman force-pushed the marketplace-tags-cleanup branch from 8cdcba2 to ea04c4f Compare June 2, 2024 02:51

rosschapman commented Jun 2, 2024

View reviewed changes

rosschapman changed the title ~~WIP~~ Marketplace Tags enhancements Jun 2, 2024

Delete, rename, and more

335fe51

rosschapman force-pushed the marketplace-tags-cleanup branch from f202d23 to 335fe51 Compare June 2, 2024 04:28

rosschapman marked this pull request as ready for review June 3, 2024 16:02

rosschapman commented Jun 3, 2024

View reviewed changes

zspencer approved these changes Jun 3, 2024

View reviewed changes

anaulin reviewed Jun 3, 2024

View reviewed changes

app/components/marketplace/tag/display_component/display_component.yml Outdated

Copy link

Member

anaulin Jun 3, 2024

Choose a reason for hiding this comment

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

Can be deleted.

Removes unused files, unnecessary turbo stream, changes copy

ca4d29e

rosschapman merged commit 64d7aa7 into main Jun 3, 2024
3 checks passed

rosschapman deleted the marketplace-tags-cleanup branch June 3, 2024 21:34

rosschapman mentioned this pull request Jun 3, 2024

Marketplace Menus: 🪩🥂🍾✨ and next steps #2426

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Marketplace Tags enhancements #2431

Marketplace Tags enhancements #2431

rosschapman commented Jun 1, 2024 •

edited

Loading

rosschapman Jun 2, 2024 •

edited

Loading

zspencer Jun 3, 2024

rosschapman Jun 3, 2024

anaulin Jun 3, 2024

rosschapman Jun 3, 2024

zspencer Jun 3, 2024

rosschapman Jun 3, 2024

anaulin Jun 3, 2024

rosschapman Jun 3, 2024 •

edited

Loading

zspencer Jun 3, 2024

rosschapman Jun 3, 2024

anaulin Jun 4, 2024

rosschapman Jun 5, 2024

rosschapman Jun 3, 2024

zspencer left a comment

zspencer Jun 3, 2024

zspencer Jun 3, 2024

zspencer Jun 3, 2024

zspencer Jun 3, 2024

zspencer Jun 3, 2024

zspencer Jun 3, 2024

rosschapman Jun 3, 2024

anaulin Jun 3, 2024

rosschapman commented Jun 3, 2024

Marketplace Tags enhancements #2431

Marketplace Tags enhancements #2431

Conversation

rosschapman commented Jun 1, 2024 • edited Loading

rosschapman Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosschapman Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zspencer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosschapman commented Jun 3, 2024

rosschapman commented Jun 1, 2024 •

edited

Loading

rosschapman Jun 2, 2024 •

edited

Loading

rosschapman Jun 3, 2024 •

edited

Loading