Feature tags step 1 #4884

coalest · 2024-12-22T12:01:55Z

Resolves #4820

Description

Add the ability to tag product drives. In a manner that will be expandable to tagging other entities.
When adding a tag, the current tags should be shown for selection, but another can be specified.
Add tagging to the product drive search.
tags are organization-specific
tagging entity will have (tag name, org id), entity type and entity id
the tagging filtration will show the tags for that kind of thing (in this case product drives) for that organization

Type of change

New feature

How Has This Been Tested?

Added new tests

Screenshots

coalest · 2025-01-07T15:46:13Z

Requesting a review from you @cielf first as I want to make sure this implementation is functionally correct.

cielf · 2025-01-07T23:13:24Z

I'm including in @dorner for parallel - and I might not get to it til the end of the week.

coalest · 2025-01-08T10:57:16Z

app/models/product_drive.rb

@@ -44,7 +45,7 @@ def end_date_is_bigger_of_end_date
    return if start_date.nil? || end_date.nil?

    if end_date < start_date
-      errors.add(:end_date, 'End date must be after the start date')


Fixes this duplicate text issue:

Unrelated change, so I can move it to another PR if needed.

coalest · 2025-01-08T10:59:07Z

Gemfile

+  # More concise test ("should") matchers
+  gem "shoulda-matchers", "~> 6.2"


Moved this gem because otherwise rails g model fails because shoulda-matcher is needed for factory bot to create the spec factory files, so we need shoulda-matcher in the dev environment as well.

cielf

Looks pretty good at first go-through.

dorner · 2025-01-10T01:13:37Z

Sorry for the delay - I keep leaving this to last in my reviews because I have to think about it the most, and I always end up without enough time to address it :) Hopefully soon!

dorner

So happy this is getting started! I think we need to tweak the solution a bit though.

dorner · 2025-01-16T00:05:47Z

app/controllers/product_drives_controller.rb

@@ -66,9 +69,9 @@ def show

  def update
    @product_drive = current_organization.product_drives.find(params[:id])
-    if @product_drive.update(product_drive_params)
+    @product_drive.attributes = product_drive_params.merge(tags: tags_from_params)
+    if @product_drive.save


Is there a reason this pattern is different than in create?

Not sure why i did that. Fixed in dbe77d7 to be more consistent with other update actions.

dorner · 2025-01-16T00:06:55Z

app/models/concerns/taggable.rb

+    private
+
+    def set_organization_id(tagging)
+      tagging.organization_id ||= organization_id


Why does the join table need an organization_id?

Also this method is pretty broadly named for something that's going to be in a model... maybe rename it so it's specific to tagging?

I think there might be a modelling issue here. It's the Tag that should belong to the organization, not the Tagging. If we want to show only product drive tags, that should be an enum on the Tag record, not the Tagging. Tagging should be a simple join table between a "taggable" and a Tag.

Either the tags table or the taggings join table need a foreign key reference to an organization. Functionally I think either would work. To me, it makes more sense that a tagging belongs to an organization rather than a tag. Because whoever is doing the tagging will belong to a certain organization so a tagging without an organization doesn't really make sense, whereas a tag doesn't necessarily need to belong to an organization it could be used by multiple organizations. If taggings references the organization, then tags can be referenced between organizations and this reduces the number of tag records. Adding type to the tag model also means more duplicates if for example there is a donation tag and a product drive tag both use the same tag name.

I know it doesn't really matter but out of curiosity I checked and it looks like the acts_as_taggable_on gem also uses a similar schema.

I think the first case is a reasonable one.

@cielf do you want to weigh in - am I right in my assumptions here?

Ok -- I'm going to muse a bit here. We definitely need to discuss this more before making any changes based on these musings.

What's in my head, here, certainly wasn't explicit in the issue. Nor was the idea of typed tags.

We want the tags to be a tool the banks can use for identifying groups of things that belong together that we haven't otherwise grouped for them.

The use case that prompted this is a "campaign" for product drives - the idea of linking product drives with a common theme, like Mothers' Day or Christmas. I could certainly see having distributions associated with either of those examples.

I can also imagine banks using this to tag things associated with particular grants -- purchases, eligible partners or partner groups, special items, distributions.

So, from a user's point of view, why do we have type associated with the tags, at least, yet?

I've added organization_id to the tags table as requested, so an organization can create/edit/delete tags independently of them being used to tag something.

Only remaining question is whether the tags table should have a type or not. We can either wait until that is a requirement in the future or I can add it to this PR. Up to y'all

We discussed during the meeting. Tags should have a type. Taggings should not need to have one since it has the taggable_id / taggable_type for the polymorphic association.

Fixed in 0cd6971.

dorner · 2025-01-16T00:07:45Z

app/models/organization.rb

@@ -68,6 +68,9 @@ class Organization < ApplicationRecord
    has_many :purchases
    has_many :requests
    has_many :storage_locations
+    has_many :taggings
+    has_many :product_drive_tags, -> { merge(Tagging.by_type("ProductDrive")).distinct },


So here this should be a simple has_many instead of :through

Is the refactor from 413a6e0 what you meant?

Nope - see above. I meant moving organization_id to tags.

Fixed in 8b7242d.

dorner · 2025-01-16T00:11:48Z

app/models/tag.rb

+class Tag < ApplicationRecord
+  has_many :taggings, dependent: :destroy
+
+  scope :alphabetized, -> { order(:name) }


...and this should have the by_type scope instead of Tagging.

Moved by_type scope to Tag in 413a6e0.

dorner · 2025-01-16T00:12:24Z

app/views/product_drives/index.html.erb

@@ -103,6 +107,7 @@
                  <td><%= product_drive.name %></td>
                  <td><%= product_drive.start_date.strftime("%m-%d-%Y") %></td>
                  <td><%= product_drive.end_date&.strftime("%m-%d-%Y") %></td>
+                  <td><%= product_drive.tags.map(&:name).sort.join(", ") %></td>


This feels like a method that should live on the Taggable concern.

True. Fixed in 99f4f97.

db/migrate/20241221180952_create_tags.rb

dorner · 2025-01-16T00:13:42Z

spec/factories/tags.rb

+FactoryBot.define do
+  factory :tag do
+    name { "Holidays" }
+    initialize_with { Tag.find_or_create_by(name:) }


I'm not the biggest fan of quietly changing this from "create new tag" to "find existing tag"... certainly not in the base factory. I'd rather have helper methods to work with this if needed.

True, I wanted to avoid issues from tags being created in tests with non unique names, but this wasn't the right way to achieve that.

Fixed in efd015f.

dorner · 2025-01-16T00:14:17Z

spec/requests/product_drives_requests_spec.rb

@@ -47,6 +47,23 @@
        expect(response.body).not_to include(product_drive_path(product_drive_five.id))
      end

+      it "allows filtering by tag" do


Should this be a shared example?

Do you mean a shared example for filtering product drives? Or a shared example for filtering different resources by tags?

Seems premature to me to create a shared example for filtering by tags, since only product drives have tags.

But the whole point of the tag feature is to make it a generic way to attach tags to anything - we know we're not going to limit it to product drives.

Ok then I will make it a shared example.

Fixed in 374dea1.

coalest · 2025-01-19T14:24:16Z

@dorner Refactored organization_id to the tags table.

dorner

Minor quibble with a name.

@cielf this looks good to me now - do you want to take one more look on the QA side since the implementation has changed?

dorner · 2025-01-21T23:35:20Z

app/models/concerns/taggable.rb

+    has_many :taggings, as: :taggable, dependent: :destroy
+    has_many :tags, through: :taggings, source: :tag
+
+    scope :by_tags, ->(tags) { left_joins(:tags).where(tags: {name: tags}) }


Can we name the parameter tag_names? It was a bit confusing till I saw where it was used.

Fixed in 4561a63.

cielf · 2025-01-22T00:38:01Z

@dorner Yup -- will try to take a look at it tomorrow.

cielf

Gave it another test. I don't know if I checked this out the first time, but if you have an error, the tags are not showing properly.

You can't put in the tags.
And, if you then try to save, you get a 500 (unless you clear the tags field)

coalest · 2025-01-22T17:53:09Z

Whoops, that was working at some point.

I just pushed commits to fix the issue with corresponding request specs.

@cielf It is ready for testing again.

cielf

I seem to be stopping as soon as a notice a problem. This time...

Any other field, if you click to a different field, the information stays.

Not so with tags.

So, for example, if you type in your tag and hit submit without hitting enter first, you lose it. Or if you type in your tag, notice a problem in the date and go to fix that, the tag disappears.

Is that something we can reasonably address?

coalest · 2025-01-23T21:29:00Z

@cielf Fixed in my last commit. That is configurable by select2, so I just had to set selectOnClose to true.

Here are the other configuration options for this dropdown in case you're curious: https://select2.org/configuration/options-api.

cielf

Hmm, Try this sequence. New product drive, don't enter a name, but enter the start date. In tags: "tagname1", hit enter, "tagname2", hit enter, "tagname3" then click save without hitting enter.
What I'm seeing in this case is that the tags go away.

coalest · 2025-01-24T18:51:03Z

I think what you might be experiencing is that new tags (ones that haven't yet been saved to a product drive) don't persist after a validation failure. That's because when the page re-renders the tag dropdown options are created from all currently existing product drive tags. Which since the validation failed, that product drive tag didn't get created and therefore doesn't appear any more.

Is this considered an edge case? Or should i fix it?

To fix it, I see two options:

Just create the tags regardless of whether the product drive validation passes. Pro -> easy fix. Con -> could lead to unwanted tags created.
Merge the tags that were attempted to be created with the currently existing tags. Pro -> tags dont get created unless product drive update succeeds. Con -> a little bit messier logic in the controller.

cielf · 2025-01-24T19:27:22Z

Well, I don't think 1/ is any worse than if they had typed in those tags on a successful product drive creation, so I'm good with that. Honestly, the most likely error is someone mistyping a end date.

coalest · 2025-01-24T19:54:06Z

@cielf Ok in that case, I just pushed a commit that implemented option 1. Ready for testing again.

cielf · 2025-01-26T17:40:51Z

LGTM. @dorner do you want to take a quick look at the latter changes?

coalest added 6 commits December 22, 2024 13:02

Fix model spec generation error

f1dcf34

Add tags and taggings model/concern

78e3fcf

Add uniqueness constraint on taggings and fix indexes

b90d5d0

Add tagging related associations and scopes

477c664

Add tag and tagging model specs

f758a9b

Add tag filtering to product drives index page

50bea61

coalest force-pushed the 4820-feature-tags-step-1 branch from 7fc06ef to 50bea61 Compare December 22, 2024 12:02

coalest added 4 commits December 22, 2024 13:08

Clean up by_tags scope

d659a35

Fix factory_bot linting error

3649fff

Merge with main

134df2b

Add ability to tag product drives

a5c8808

coalest force-pushed the 4820-feature-tags-step-1 branch from a0c0bb1 to a5c8808 Compare January 6, 2025 17:30

coalest changed the title ~~WIP: Feature tags step 1~~ Feature tags step 1 Jan 6, 2025

coalest added 6 commits January 6, 2025 18:31

Merge branch 'main' into 4820-feature-tags-step-1

19788eb

Remove WIP controller

af09236

Fix factory bot lint error

b0bcd59

Fix missing tags after validation failure

5781e21

Add specs

788f37e

Fix styling of product drive modal

b1a1e28

coalest requested a review from cielf January 7, 2025 15:46

cielf requested a review from dorner January 7, 2025 23:12

coalest commented Jan 8, 2025

View reviewed changes

cielf reviewed Jan 8, 2025

View reviewed changes

dorner requested changes Jan 16, 2025

View reviewed changes

coalest added 2 commits January 16, 2025 11:46

Add tags_for_display method to Taggable concern

99f4f97

Fix name in tag factory

efd015f

Move by_type scope spec to tag model

f693485

coalest requested a review from dorner January 16, 2025 11:28

coalest added 3 commits January 16, 2025 13:09

Order tags alphabetically in dropdowns

764e9ba

Move organization_id to tags table

8b7242d

Fix linting

c1ba4c0

coalest added 5 commits January 20, 2025 14:15

Refactor filter by tags to shared example

374dea1

Remove unnecessary scope

729562b

Fix wip changes

ab375e0

Merge branch 'main' into 4820-feature-tags-step-1

2285747

Add type column to tags table

0cd6971

dorner requested changes Jan 21, 2025

View reviewed changes

Rename by_tags scope parameter to tag_names

4561a63

cielf requested changes Jan 22, 2025

View reviewed changes

coalest added 3 commits January 22, 2025 18:31

Fix missing tags on failure bug

a39e074

Fix lost tags on product drive update failure

801fe71

Add request specs for preserving tags on validation failure

9cfc96c

coalest requested a review from cielf January 22, 2025 20:04

cielf requested changes Jan 22, 2025

View reviewed changes

Set selectOnClose config value to true for product drive tags

f8b773f

coalest requested a review from cielf January 23, 2025 21:29

cielf requested changes Jan 24, 2025

View reviewed changes

Fix new tag disappearance on validation failure

6e672a4

		# More concise test ("should") matchers
		gem "shoulda-matchers", "~> 6.2"

Feature tags step 1 #4884

Are you sure you want to change the base?

Feature tags step 1 #4884

Conversation

coalest commented Dec 22, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Screenshots

coalest commented Jan 7, 2025 • edited Loading

cielf commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cielf left a comment

Choose a reason for hiding this comment

dorner commented Jan 10, 2025

dorner 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

cielf Jan 18, 2025 • 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

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

Choose a reason for hiding this comment

coalest Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coalest commented Jan 19, 2025

dorner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cielf commented Jan 22, 2025

cielf left a comment • edited Loading

Choose a reason for hiding this comment

coalest commented Jan 22, 2025 • edited Loading

cielf left a comment

Choose a reason for hiding this comment

coalest commented Jan 23, 2025

cielf left a comment

Choose a reason for hiding this comment

coalest commented Jan 24, 2025 • edited Loading

cielf commented Jan 24, 2025

coalest commented Jan 24, 2025

cielf commented Jan 26, 2025

coalest commented Dec 22, 2024 •

edited

Loading

coalest commented Jan 7, 2025 •

edited

Loading

cielf Jan 18, 2025 •

edited

Loading

coalest Jan 17, 2025 •

edited

Loading

cielf left a comment •

edited

Loading

coalest commented Jan 22, 2025 •

edited

Loading

coalest commented Jan 24, 2025 •

edited

Loading