Detect enum fields as Selects rather than as Strings

[tadeusz-niemiec]

Fixes #1629
Improve enum field definition in the dashboard generator

[tadeusz-niemiec]

This fails due to the vulnerabilities found in 6.0.2.2 version of some action gems, not sure if it's ok to bump them in this PR.

[stevenbuccini]

Thanks for putting this up, just ran into this problem and it was really nice to see that someone had taken the time to make it better.

[pablobm]

Don't worry about the vulns. We just upgraded some gems, so a rebase might be enough.

[pablobm]

Regarding the PR, I think it could do the trick, although here's another potential idea.

Since #1589 (not yet released in the gem) it's possible to pass a call-able as collection, for example a lambda. Therefore I wonder if it would be possible to use ATTRIBUTE_TYPE_MAPPING, and then ATTRIBUTE_OPTIONS_MAPPING with something that generates the values.

There's one problem I see: at the moment, call doesn't pass any arguments, so it's not possible to know anything about the field inside the lambda. I think it would be reasonable to pass the field instance at the following location:

administrate/lib/administrate/field/select.rb

Line 18 in 204e0ab

return maybe_proc.call if maybe_proc.respond_to? :call

Then we can have the field be something like this:

state: Field::Select.with_options(
  collection: ->(field) { field.resource.class.send(field.attribute.to_s.pluralize).keys) },
)

OK, I'm not 100% sure of what I'm saying... The good thing about your approach is that it impacts only the generator, so no interfaces have to change. I have to think about this.

Thoughts anyone? /cc @nickcharlton

[tadeusz-niemiec]

@pablobm alright, I think I figured it out. The only problem here is that it's pretty tricky to fetch Proc's definition as a String. The easiest way is to read the definition line from a file and parse it, but this forces us to define Procs in one line (or write a really good parser for multi-liners). The other solution would be to add a external gem to handle this, but it's an additional dependency.

Please let me know what you think about this.

(I have to violate the line length rule in ATTRIBUTE_OPTIONS_MAPPING since I use one-line definition parser)

[nickcharlton]

A few style comments; I don't find myself with many implementation thoughts, so I'll defer to @pablobm here as he's already thinking about it.

[nickcharlton]

We can drop "properly" on this descriptor.

[nickcharlton]

It's a little thing, but I wonder if we can use the lamdba do; end syntax here to achieve the same goal?

[tadeusz-niemiec]

It all depends on how good the parser will be, because I fetch raw file line there and parse it using regexp. We'll see how this idea works out and I'll try to implement parser for all possible syntaxes if it's the way to go.

[nickcharlton]

Can you add the the deleted line back in?

[hound]

Metrics/LineLength: Line is too long. [108/80]

[pablobm]

OK, I think I prefer your approach to what I mentioned. Your change only affects generators and doesn't change the internal APIs. It'll be safer particularly as we are starting to think about changing those APIs and we'll want to be careful about it.

If you can address that last couple of comments, I think we can merge this.

[tadeusz-niemiec]

OK, I think I prefer your approach to what I mentioned. Your change only affects generators and don't change the internal APIs. It'll safer particularly as we are starting to think about changing those APIs and we'll want to be careful about it.

Do you mean that I should revert this to the previous version? Because actually it does what you mentioned, but imho it's pretty safe - the only difference in the select field is one ternary operator to check if proc requires any arguments.

I've upgraded my regexp so it will handle do |arg|; block end definitions in the current solution.

btw some vulns were found

[nickcharlton]

Kaminari's update was merged in #1659 so if you rebase against current master it'll solve that vulnerability.

[pablobm]

Oh, I see 😅 Sorry, I should have paid more attention.

Hm, I have to admit I'm easy. Your change (sending the field as an argument) actually looks good, and I think it's the reasonable thing to do. I don't think we'd have done differently in the future.