Initial tabbed Primo/TIMDEX interface

[jazairi]

Why these changes are being introduced:

USE UI needs affordances to switch between results from different sources. Primo results should be
the default view.

Relevant ticket(s):

• USE-31
• TIMX-549

How this addresses that need:

• Integrates Primo search into the controller and view layers
• Adds Turbo frame 'tabs' to switch between Primo and TIMDEX results.

Side effects of this change:

• Maintaining separate views for each result type is probably not ideal, but I'm accepting it as a
  risk until we normalize TIMDEX records similarly
  to how we normalize Primo.
• Several tests have been skipped for features that are no longer relevant to USE UI, but are
  core to GDT. We will need to revise our overall
  test strategy such that these features are tested
  as part of GDT.
• FRBRized full record links don't always work. It the method we use breaks FRBR links for CDI
  records. Predictably, Ex Libris documentation on
  FRBR does not address this issue. We might decide
  to forgo FRBR links in general, or perhaps limit
  them by content type (assuming book results always come from Alma).

Developer

Accessibility

• ANDI or WAVE has been run in accordance to our guide.
• This PR contains no changes to the view layer.
• New issues flagged by ANDI or WAVE have been resolved.
• New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
• No new accessibility issues have been flagged.

New ENV

• All new ENV is documented in README.
• All new ENV has been added to Heroku Pipeline, Staging and Prod.
• ENV has not changed.

Approval beyond code review

• UXWS/stakeholder approval has been confirmed.
• UXWS/stakeholder review will be completed retroactively.
• UXWS/stakeholder review is not needed.

Additional context needed to review

• While testing changes, please enable GDT in env to ensure that GeoData functionality is not disrupted.
• Note that pagination is not fully implemented here in an attempt to make a large changeset a little smaller. However, it can be added if the reviewer feels otherwise.

Code Reviewer

Code

• I have confirmed that the code works as intended.
• Any CodeClimate issues have been fixed or confirmed as
  added technical debt.

Documentation

• The commit message is clear and follows our guidelines
  (not just this pull request message).
• The documentation has been updated or is unnecessary.
• New dependencies are appropriate or there were no changes.

Testing

• There are appropriate tests covering any new functionality.
• No additional test coverage is required.

[coveralls]

Pull Request Test Coverage Report for Build 18326089153

Details

• 41 of 43 (95.35%) changed or added relevant lines in 1 file are covered.
• 5 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.8%) to 98.126%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
app/controllers/search_controller.rb	41	43	95.35%

Files with Coverage Reduction	New Missed Lines	%
app/helpers/application_helper.rb	2	92.0%
app/controllers/search_controller.rb	3	95.21%

Totals
Change from base Build 18320550946:	-0.8%
Covered Lines:	838
Relevant Lines:	854

---

💛 - Coveralls

[matt-bernhardt]

Nothing here is blocking, but I did want to ask a few questions to confirm what I think is your intent. There's a few things that I think might warrant follow-up later, but no reason not to merge and move forward for this sprint.

• One thought about a possible streamlining of logic in the search controller (might warrant a future ticket, or to influence success criteria if I ticket touches the results method).
• One question to confirm what I think is your approach to the load_* methods (which I think I agree with, if I'm right)
• One possible point to return to when we remove FlipFlop and how that'll change results.html.erb
• One possible point to return to when we get real feedback from TACOS and update how the fact panels work
• One question about flipflop in tests, which doesn't need to be changed now considering we're going to be removing FlipFlop soon anyway.

- from the tests and poking I've done locally, this seems to be okay. Thanks!

[matt-bernhardt]

Non-blocking, but I'm not sure I understand something - given that there's a pathway above to assign @active_tab to have a value of gdt, is there a reason why we wouldn't then use a simple case statement below, rather than a case inside an if/else?

[jazairi]

That's the end game, but I was saving that change for when we rip out FlipFlop. For now, it made sense to me to stick with our current mechanism for GDT feature flagging, even if the code is hideous.

[matt-bernhardt]

Non-blocking question: I see that the first line of all the load_* methods is what looks like the same call to Enhancer.new().enhanced_query. My assumption is that your goal is to have as clean a results method as possible, rather than one cluttered with stock steps and then conditionals for only those parts of the workflow that diverge?

I can get behind that motivation, I think, but will confess there was an initial urge to go the other way, and keep the common parts of these methods up in results.

[jazairi]

Good point. I think we could move that @enhanced_query assignment, which is shared across the three methods, to the results method. As for the other similarities, load_gdt_results is basically the same as load_timdex_results, except it extracts filters. I decided to make these separate methods because it makes it clearer (to me) what's going on, but we could collapse them into one and include an @active_tab check to determine whether to display filters.

[matt-bernhardt]

I don't think this is blocking, but it took me drawing a four-square diagram to parse what conditions were possible and what the response would be here. If I've done this correctly, then any errors will cause this panel to simply be blank (while the errors partial up on line 27 will be showing the error contents) - but if not, then the old behavior of "results or no-results message" still applies.

I wish this could be simpler - but in some sketching while reviewing this, I'm not sure that's easily achievable in this PR. Do you think this could either be a ticket for when we're polishing the UI, or something to check back in on after Dave's made more progress with a real interface?

[jazairi]

I agree. The 'easier' way I found was something like this:

<% if @errors.blank? %>
  <!-- Do nothing -- errors rendered in partial above -->
<% elsif @results.present? %>
  list results, etc

It's easier to read, but it felt worse. The other option is to move where errors are rendered into the main element where the results would be:

<% if @errors.present? %>
  <%= render(partial: 'shared/error', collection: @errors) %>
<% elsif @results.present? %>
  list results, etc
<% else %>
  <div class="no-results">
    <p class="hd-2">No results found for your search</p>
  </div>
<% end %>

While this leads to a much cleaner conditional, errors rendered in that section of the DOM look odd to me. So, this felt like a 'least bad' kind of solution.

[matt-bernhardt]

I see these working, and they work because the fact panels work independently of what Primo/TIMDEX do, but something about this change (from a cassette that has a search related to this test to a generic mocked successful search) makes me uneasy.

I'm not asking for a change, because it also seems wasteful to have custom mocks for a feature that fundamentally doesn't care what Primo/TIMDEX does. Maybe this will become cleaner after we're getting these fact panels from TACOS.

[jazairi]

I hear you. I find that generic mocks make sense here partly because the feature is search-agnostic. But I agree that it would be worth revisiting this and TACOS integration both land, in case there's a better way to test it.

[matt-bernhardt]

I'm not clear how to trigger this into the true state like this - I know I can change env (or via ClimateControl), but I'm not clear how to affect FlipFlop in a test like this. It ends up being skipped every time. If there's something I need to learn about flipflop, I'm all ears.

[jazairi]

As we discussed in Slack, this is basically a fancy skip. It felt out of scope to rewrite a bunch of GeoData tests in this changeset. Still, I wanted to retain those assertions, because when we do get rid of FlipFlop, it would be good to keep that coverage for GeoData. But yes, you're right that this is not really how we use FlipFlop. (Though, it would be cool to have a make run-geodata-tests type of command that flipped the feature and just ran the GDT tests!)

[jazairi]

@matt-bernhardt Thanks as always for the thoughtful feedback! I made a couple of changes based on your comments and responded to those that I didn't change. I'm hopeful that most of the remaining suggestions will be resolved when we move away from FlipFlop, but there are a couple others that we may want to address here or revisit later.

[matt-bernhardt]

Your answers make sense to me - thanks for making the changes you did. Looking forward to seeing how this all changes we move into this other work.

(edited to add - it looks like there's now a conflict in the search controller test, because the TACOS integration merged. I'm happy to take a look at this if you'd like, but it isn't required - looking at the conflict I don't think there's anything bad lurking here, I just changed some assertions to add a class value to make them unique to the fact panels)