Adds possibility to define allowed values for tags #40

jplethier · 2022-06-16T18:53:33Z

Proposal

This PR proposes to have a easy way to add simple validations for tags allowed to model using this gem

Changes

Adds an allowed option as parameters for acts_as_taggable_array_on to define allowed values for tags
Adds a validation method when the allowed param is present and is valid to ensure that all tags values are allowed
Adds a check to ensure that the allowed param is an array when it is present, raising an error if it is not an array

skatkov · 2022-06-22T22:53:23Z

Thanks for your contribution.

Can you please rebase your changes and update README accordingly?

New updates

…s inclusion on this list

jplethier · 2022-06-24T11:18:32Z

@skatkov I've just updated the branch with all new changes and updated readme adding information about allow list option. Please review the readme and give me any feedback if it can be more clear on better in any other way

skatkov · 2022-06-26T15:27:26Z

spec/acts_as_tag_pgarray/taggable_spec.rb

+    context "when options is not an array" do
+      it "raises an error" do
+        expect { User.acts_as_taggable_array_on :colors, allow_list: "red, blue" }
+          .to raise_error(described_class::InvalidAllowListTypeError)


I've noticed, that your adding an .error to AR object. Maybe it's good to test that? and make sure that .valid? returns "false"?

I don't know if I understand correct the suggestiont, but this specific tests is about tag definition, not about the active record validation. I just added error to be raised when the tags definition is invalid, in this case, the allow list is invalid. Does that make sense?

skatkov

Thanks for your contribution—I've added some comments and questions. So far, I don't feel comfortable merging this MR as is.

But definitely see this as a valuable addition to a gem and would like to work out details to get this merged.

README.md

skatkov · 2022-06-26T15:44:53Z

spec/acts_as_tag_pgarray/taggable_spec.rb

+
+    context "when options is an array" do
+      before do
+        User.acts_as_taggable_array_on :colors, allow_list: %w[red blue]


Would be interesting to know, what would happen in case of any other datatype passed to allow_list. How can we handle that better?

e.g. string or number:
taggable_array :colors, allow_list: 'red'

or hash:
taggable_array :colors, allow_list: {red: 'red'}

Also would be interesting to know, how would we comparse arrays with strings inside and symbols. I would expect that ['red'] would equal [:red]

Can we add a test to make sure, that this is a case?

Would be interesting to know, what would happen in case of any other datatype passed to allow_list. How can we handle that better?

e.g. string or number: taggable_array :colors, allow_list: 'red'

or hash: taggable_array :colors, allow_list: {red: 'red'}

About this, there is a check to guarantee that allow list is an array, and if it is not an array, it throws an error saying that allow list must be an array. There is a test for this too https://github.com/tmiyamon/acts-as-taggable-array-on/pull/40/files#diff-ad2deaee3a6d67e0e4d46e72478cb4ea6fc8209a10fa2c244656f3683f2803bcR166

Also would be interesting to know, how would we comparse arrays with strings inside and symbols. I would expect that ['red'] would equal [:red]

Can we add a test to make sure, that this is a case?

I added tests scenarios to this and make sure to deal with those two options as the same

skatkov · 2022-06-26T22:40:53Z

lib/acts-as-taggable-array-on/taggable.rb

@@ -18,6 +28,16 @@ def acts_as_taggable_array_on(tag_name, *)
        scope :"without_any_#{tag_name}", ->(tags) { where.not("#{table_name}.#{tag_name} && ARRAY[?]::#{tag_array_type_fetcher.call}[]", parser.parse(tags)) }
        scope :"without_all_#{tag_name}", ->(tags) { where.not("#{table_name}.#{tag_name} @> ARRAY[?]::#{tag_array_type_fetcher.call}[]", parser.parse(tags)) }

+        if args[:allow_list].present?


Can we extract :allow_list into a constant?

It seems reasonable to do, since we repeat this at least 3 times in method definition. In case we need to switch this value, we'll have lower chance of typo mistake.

If I understand correctly what you suggested, I did in this commit 8c1aa9d

skatkov · 2022-06-26T22:47:25Z

spec/acts_as_tag_pgarray/taggable_spec.rb

-      expect(User.without_any_colors("red, blue")).to match_array([@admin1])
-    end
-  end
+  context "without allow_list" do


Top level context has become:

"without allow_list"

(all other cases)

"with allow_list"

(all other cases)

I don't feel that "allow_list" is such an important feature to warant top ranking in testsuit hierarchy. Can we append new testsuit for new feature to previous hierarchy? Example

(all other cases)

#allow_list

without allow_list

(tests)

with allow_list

(tests)

what do you think to something like:

without any configurations

with configurations

#allow_list

I put all the other cases inside a context to be sure that the setup for those other cases does not pollute the test scenarios for allow list and vice versa. If I just remove the context and let all other cases direct in the top level, it will run a lot of setup code on before for the allow list tests that are not necessary, won't be useful and can generate unintended results. Even thinking about test performance, adding more options, and having this top-level cases running every time is not good.

I considered writing the validations tests(allow list now, and maybe a block list, quantity limit, and more in the future) in a different file, so we would have all normal and top case scenarios here in this file, and, for now, allow list tests in another file. What do you think of this approach?

…ating tags allowed

jplethier · 2022-06-28T23:53:21Z

@skatkov thanks a lot for all the feedback and suggestions. I did some of them already, and I am in doubt about the others ones, that I answered you here.

skatkov

Nice work so far. I don't have too much time to contribute to open source these days.

So while kid is sleeping, decided to give this PR a review. If we're planning to change API of this gem, i'd love to polish it up more!

I've shared some of my thoughts on proposed changes. Some of those thoughts are pretty blury :P

skatkov · 2022-07-03T22:27:38Z

lib/acts-as-taggable-array-on/taggable.rb

@@ -2,14 +2,24 @@

 module ActsAsTaggableArrayOn
  module Taggable
+    class TaggableError < StandardError; end
+
+    class InvalidAllowListTypeError < TaggableError


It's nice to see that your error handling, it's really useful.

I would like to consider if all of this code to raise an error is actually necessary. Maybe there is a way to simplify and improve API in here?

We basically have two additional errors right now, they are:

ActsAsTaggableArrayOn::Taggable::TaggableError ActsAsTaggableArrayOn::Taggable::InvalidAllowListTypeError

Not really readable, though? "InvalidAllowListTypeError" reminds me of my java development experience, but we can do better in ruby :P

We use only one error in our code, though. Why do we define two types of errors?

One of errors has a .to_s method, but second one doesn't. Should we provide a way to translate this definition? Do we really need it?

What kind of errors do ActionRecord/ActiveModel throws? Would it be better to use that instead?

I've looked into some possible errors from ActiveRecord, and it seems reasonable to inherit from ActiveRecord::ActiveRecordError. But can we reuse one of errors from ActioveRecord namespace or should we define our own? I haven't found anything suitable for our cause in existing list off errors.

I would propose to use ActiveRecord namespace for our errors, e.g: And define a single error only.

class ActiveRecord::<error_name> < ActiveRecord::ActiveRecordError; end

Regarding a name, I propose something simpler, maybe TagDefinitionError or maybe TaggableArrayError?

skatkov · 2022-07-03T22:39:40Z

spec/acts_as_tag_pgarray/taggable_spec.rb

+  context "with allowed option" do
+    context "when options is not an array" do
+      it "raises an error" do
+        expect { User.acts_as_taggable_array_on :colors, allowed: "red, blue" }


What should happen if allowed is an empty array? Can we test for that as well?

Good catch. What do you think about throwing an error in this scenario? Another option would be to allow any value, having the same behavior as when the allowed option is not present.

I allowed only has as an empty array, just act like there is no such setting. No need to throw errors.

skatkov · 2022-07-03T22:43:58Z

spec/acts_as_tag_pgarray/taggable_spec.rb

-      expect(User).to respond_to(:without_all_colors)
-    end
-  end
+  context "without allowed option" do


Can you please remove this context? I don't see any need for this to be in a root level context for testsuite.

Did you read my previous comment about this? What do you think about moving all validations tests scenarios to another file?

skatkov · 2022-07-03T22:47:10Z

spec/acts_as_tag_pgarray/taggable_spec.rb

+        shared_examples "allowed with varchar array" do
+          it "accepts allowed values as strings" do
+            expect(User.new(colors: %w[blue red])).to be_valid
+            expect(User.new(colors: %w[red])).to be_valid


Your testing for array with symbols, string or integer values. But what if anything else will be provided? A hash like object inside array? Any other object? class name?

I think that we could throw an exception in this case, what do you think?

I think that we can go forward now supporting only these three kinds of types, integers, strings and symbols, do you agree?

Let's start with just integers, strings, symbols inside an array.

It's probably possible to check if any other object is serializable, but let's not go that far for now.

skatkov · 2022-07-03T23:02:02Z

lib/acts-as-taggable-array-on/taggable.rb

+      def define_allowed_validations!(tag_name, allowed)
+        raise InvalidAllowListTypeError if !allowed.is_a?(Array)
+
+        remove_const "#{tag_name.upcase}_ALLOWED" if const_defined?("#{tag_name.upcase}_ALLOWED")


I'm afraid, that my previous comment about constant was incorrectly phrased and understood. Sorry about that.

Would be awesome not to make this impementation detail part of public api for this gem. Can we hide this allowed variable inside a private method?

I'm not most used to this meta-programming method definitions in ruby, but I tried to apply your suggestion here d5c44f1

jplethier · 2022-07-04T01:14:02Z

Nice work so far. I don't have too much time to contribute to open source these days.

So while kid is sleeping, decided to give this PR a review. If we're planning to change API of this gem, i'd love to polish it up more!

I've shared some of my thoughts on proposed changes. Some of those thoughts are pretty blury :P

I know the feeling, I have two kids, and most of work on open source or personal projects is done before they awake or after they go to bed. :D

Thanks a lot for the review and the suggestions.

jplethier · 2022-10-10T23:16:12Z

@skatkov were you able to take a look in the changes that I did after your suggestions?

jplethier added 4 commits June 24, 2022 07:54

Merge pull request #1 from tmiyamon/master

1b37daa

New updates

adds allow list option to acts_as_taggable_array_on and validates tag…

5878f2c

…s inclusion on this list

updates changelog

d728918

runs linter fixed

0c19da7

jplethier force-pushed the model-validations branch from a310511 to 0c19da7 Compare June 24, 2022 11:07

updates readme with allow list informations

7b2f580

skatkov reviewed Jun 26, 2022

View reviewed changes

skatkov requested changes Jun 26, 2022

View reviewed changes

skatkov reviewed Jun 26, 2022

View reviewed changes

jplethier added 5 commits June 28, 2022 07:53

defining a constant to store allowed list for tags

8c1aa9d

changed option name from allow_list to allowed

cc3d250

refactoring and moving all validation definition to its own method

56f20c8

fix allow list renaming on changelog

208cbdb

adds more tests scenarios and always compare using strings when valid…

7131145

…ating tags allowed

jplethier requested a review from skatkov June 28, 2022 23:53

skatkov requested changes Jul 3, 2022

View reviewed changes

skatkov reviewed Jul 3, 2022

View reviewed changes

jplethier added 2 commits July 5, 2022 08:56

sets allowed tag array as a private method

d5c44f1

changes error raised when allowed is not an array

788e26f

jplethier requested a review from skatkov July 13, 2022 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds possibility to define allowed values for tags #40

Adds possibility to define allowed values for tags #40

jplethier commented Jun 16, 2022 •

edited

Loading

skatkov commented Jun 22, 2022

jplethier commented Jun 24, 2022

skatkov Jun 26, 2022 •

edited

Loading

jplethier Jun 28, 2022 •

edited

Loading

skatkov left a comment

skatkov Jun 26, 2022 •

edited

Loading

skatkov Jun 26, 2022 •

edited

Loading

jplethier Jun 28, 2022

jplethier Jun 28, 2022

skatkov Jun 26, 2022

jplethier Jun 28, 2022

skatkov Jun 26, 2022 •

edited

Loading

jplethier Jun 28, 2022

jplethier commented Jun 28, 2022 •

edited

Loading

skatkov left a comment

skatkov Jul 3, 2022 •

edited

Loading

skatkov Jul 3, 2022 •

edited

Loading

skatkov Jul 3, 2022

jplethier Jul 4, 2022

skatkov Aug 10, 2024

skatkov Jul 3, 2022

jplethier Jul 4, 2022

skatkov Jul 3, 2022

jplethier Jul 4, 2022

jplethier Jul 5, 2022

skatkov Aug 10, 2024

skatkov Jul 3, 2022

jplethier Jul 5, 2022

jplethier commented Jul 4, 2022

jplethier commented Oct 10, 2022

Adds possibility to define allowed values for tags #40

Are you sure you want to change the base?

Adds possibility to define allowed values for tags #40

Conversation

jplethier commented Jun 16, 2022 • edited Loading

Proposal

Changes

skatkov commented Jun 22, 2022

jplethier commented Jun 24, 2022

skatkov Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

jplethier Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

skatkov left a comment

Choose a reason for hiding this comment

skatkov Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

skatkov Jun 26, 2022 • 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

skatkov Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplethier commented Jun 28, 2022 • edited Loading

skatkov left a comment

Choose a reason for hiding this comment

skatkov Jul 3, 2022 • edited Loading

Choose a reason for hiding this comment

skatkov Jul 3, 2022 • 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

jplethier commented Jul 4, 2022

jplethier commented Oct 10, 2022

jplethier commented Jun 16, 2022 •

edited

Loading

skatkov Jun 26, 2022 •

edited

Loading

jplethier Jun 28, 2022 •

edited

Loading

skatkov Jun 26, 2022 •

edited

Loading

skatkov Jun 26, 2022 •

edited

Loading

skatkov Jun 26, 2022 •

edited

Loading

jplethier commented Jun 28, 2022 •

edited

Loading

skatkov Jul 3, 2022 •

edited

Loading

skatkov Jul 3, 2022 •

edited

Loading