Repository navigation
Create select_taxons admin form component - #6653
benjaminwil wants to merge 3 commits into
Conversation
6747670 to
407b33a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6653 +/- ##
=======================================
Coverage 92.47% 92.48%
=======================================
Files 1065 1066 +1
Lines 21541 21558 +17
=======================================
+ Hits 19921 19938 +17
Misses 1620 1620 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2899eff to
d23c1f4
Compare
tvdeyen
left a comment
There was a problem hiding this comment.
We should use the configured routes by using the routes helpers we have available in components, via the helpers proxy.
d23c1f4 to
c77f9c6
Compare
|
I still think this is worth reviewing right now, but I have to go back and re-evaluate the tests due to timing issues. There are indications that the remote data is not being loaded fast enough for the feature tests to fail. We're not waiting in quite the right ways. |
| <%= render component("ui/forms/field").select_taxons f, | ||
| :parent_id, | ||
| include_blank: t(".none"), | ||
| multiple: false %> |
There was a problem hiding this comment.
This should be the default anyway, no?
| multiple: false %> | |
| %> |
There was a problem hiding this comment.
From a data perspective, I thought that multiple: true makes sense as a default. Since you'd almost always want a collection of taxons to be associated with something.
But maybe from a developer experience perspective you're more correct.
There was a problem hiding this comment.
@tvdeyen @jarednorman I don't feel strongly about this, but I've left it with a multiple: true default for now. This felt like the natural thing to do, due to the method name (.select_taxons plural).
If you all feel strongly in the other direction, I can change this.
c77f9c6 to
f808334
Compare
I was wrong. The issue was that using URL helpers via |
| errors:, | ||
| label:, | ||
| multiple: true, | ||
| src: SolidusAdmin::Engine.routes.url_helpers.pretty_names_taxons_url, |
There was a problem hiding this comment.
In my local testing this still caused issues outside of tests because the host wasn't set because this doesn't have a request context, but this complicates the JS.
What if we just add a taxon select component that renders our select component from the view layer and avoids all of this complexity.
There was a problem hiding this comment.
I'm not opposed to doing it that way.
But maybe it's worth discussing more? I feel like all of this is pointing to the fact that it's not easy enough to create select components that use remote data sources. Do we want to (and do we want application developers to) have to write view partials every time we (they) want a mildly complex select component in the admin interface?
I think the complexity is reasonably reduced if we can pass paths (rather than URLs) to the solidus_select.js we've built on top of Tom Select.
cc @tvdeyen
There was a problem hiding this comment.
I don't have a strong preference either way. If accepting paths makes stuff easier and is reasonably easy to do, that approach has my blessing.
There was a problem hiding this comment.
@jarednorman @tvdeyen I added a new commit that tackles this issue by allowing the use of path helpers.
I chose this route because:
- Currently the component is just a class method on the
Form::Field::Component, so there would be some indirection and a lot more code to make a new component and component template. (Said another way: less code this way.) - I think I was able to offset the complexity of getting URLs in a reasonable way, after all. See the Apply host URL to
solidus_selectdata-srcs commit.
Let me know what y'all think.
f808334 to
98bfa7d
Compare
| else | ||
| within(dropdown) { expect(page).to have_css(".option", minimum: 1) } | ||
| control.click | ||
| end |
There was a problem hiding this comment.
I don't think the second half of this branch is used in this PR and I also don't think it works.
I think we can drop the branch, always click before waiting, and even remove the janky xpath lookup above.
There was a problem hiding this comment.
It's been a minute since I wrote this, but I think you're right.
So let's see whether the CI workflows agree now...
There was a problem hiding this comment.
Re: your comment below: yeah, the timing issue is here.
I took another stab at this with less branching and less janky xpaths lookups.
98bfa7d to
f880d3d
Compare
4345aad to
5695a07
Compare
jarednorman
left a comment
There was a problem hiding this comment.
There's a race condition in admin/lib/solidus_admin/testing_support/feature_helpers.rb#51-58 that I think needs fixing as part of this.
d9e4f7b to
e0f7d56
Compare
#select_taxons admin form componentselect_taxons admin form component
e0f7d56 to
4927e78
Compare
|
Just rebased against |
|
|
||
| private | ||
|
|
||
| def options_preloaded?(dropdown) |
There was a problem hiding this comment.
I don't think this is ever returning true. Tom Select keeps the dropdown display: none until it's opened, so hidden options are never found with the default visibility and this is just burning the full wait time every time it gets called. (My testing seems to confirm this.)
I maintain that we can just always click and then do dropdown.first(".option") and avoid this complexity.
There was a problem hiding this comment.
When I do that:
- unless options_preloaded?(dropdown)
- control.click
- dropdown.first(".option")
- end
+ control.click
+ dropdown.first(".option")I consistently see the failures pasted below. Do you have any better ideas for how to force the wait we need?
Failures:
1) Product product organization adding new category creates new category and assigns it to product
Failure/Error: expect(control).to have_text(val)
expected to find text "Apparel" in ""
# ./lib/solidus_admin/testing_support/feature_helpers.rb:62:in 'block in SolidusAdmin::TestingSupport::FeatureHelpers#solidus_select'
# ./lib/solidus_admin/testing_support/feature_helpers.rb:55:in 'Array#each'
# ./lib/solidus_admin/testing_support/feature_helpers.rb:55:in 'SolidusAdmin::TestingSupport::FeatureHelpers#solidus_select'
# ./spec/features/product_spec.rb:235:in 'block (5 levels) in <top (required)>'
# ./spec/features/product_spec.rb:233:in 'block (4 levels) in <top (required)>'
2) Product product organization adding new category with invalid attributes when taxon with same name already belongs to a parent shows error
Failure/Error: expect(control).to have_text(val)
expected to find text "Apparel" in ""
# ./lib/solidus_admin/testing_support/feature_helpers.rb:62:in 'block in SolidusAdmin::TestingSupport::FeatureHelpers#solidus_select'
# ./lib/solidus_admin/testing_support/feature_helpers.rb:55:in 'Array#each'
# ./lib/solidus_admin/testing_support/feature_helpers.rb:55:in 'SolidusAdmin::TestingSupport::FeatureHelpers#solidus_select'
# ./spec/features/product_spec.rb:267:in 'block (7 levels) in <top (required)>'
# ./spec/features/product_spec.rb:265:in 'block (6 levels) in <top (required)>'
There was a problem hiding this comment.
The solution was to dropdown.first(".option", exact_text: /.+/)
| end | ||
|
|
||
| resource :taxons, only: [] do | ||
| get :pretty_names |
There was a problem hiding this comment.
Because no permissions include pretty_names on taxons, this will only work for super admins. Needs to auth on read, I think.
There was a problem hiding this comment.
Then for now I will use the same pattern we have already committed for other solidus_admin helper controller actions (like SolidusAdmin::CountriesController#states):
skip_before_action :authorize_solidus_admin_user!This better handles options that are loaded from a remote data source URL. A CI/CD timing-related failure indicated that this code was required.
We want to create a reusable taxon select component for a couple of reasons: 1. It is too error-prone to create this by hand. 2. We want to be able to create consistent functionality where ever there is a taxon select required. This means always using the `#pretty_name` method and having shared code to load and present a list of taxons. As you can see from the diff, this commit allows us to delete some duplicated and kinda weird code for creating bespoke taxon select components.
The custom Solidus Select web component does not support relative URLs as remote data sources passed to `data-src`. This is because the underlying select library, Tom Select, only allows absolute, fully- qualified URLs to be used as remote data sources. Unfortunately, I have learned that our admin components do not always have access to Rails URL helpers or default URL options before render time. This would need to be configured per application, which isn't a very friendly thing for a framework like Solidus to require. But we do know that, at render time, we can get the request information to apply implied host information to a relative URL from root. This allows us to use path helpers component-side and still get full URLs at render.
4927e78 to
98aa43a
Compare
Summary
This pull request creates a bespoke component for selecting taxons from a list of all of a store's taxons. This resolves #6643.
It also starts using the new component in two existing templates.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed:
Footnotes
There is a JavaScript console error hard-coded in English. The intended audience is developers, though, not customers. ↩