Adds support for configuring specific TIMDEX index #63
Conversation
Pull Request Test Coverage Report for Build 32b1840cfd6c9b0c962cffc3b35370bd2bcc0c43-PR-63
💛 - Coveralls |
Why are these changes being introduced: * Being able to choose an index to search is required to be able to provide an RDI specific instance of this app Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/RDI-205 How does this address that need: * Pulls in the latest schema from GraphQL which enables optional setting of the index * Provdes `TIMDEX_INDEX` value in GraphQL query if it is set in ENV, if it is not set, the TIMDEX endpoint will default to whatever default value it is configured for * Updates cassettes since the schema changed Side effect: - Analyzer#hits private method was created to allow for stubbing hits in tests to avoid having to use cassettes to test the Analyzer. This removes a fragile test. - Added mocha gem to facilitate the stubbing noted above - Some cassettes were shared between controller and model tests for Record and they now need their own cassettes because the GraphQL is slightly different (we also could have updated the model tests but this seemed easier and less fragile down the road to not couple unrelated tests to the same cassette)
JPrevost
force-pushed
the
rdi-205-allow-defining-index-in-ui
branch
from
ae653f6 to
32b3822
Compare
| @@ -2,63 +2,57 @@ | |||
|
|
|||
| class AnalyzerTest < ActiveSupport::TestCase | |||
There was a problem hiding this comment.
Thinking a bit more about this change to stubs, I think we need a test to confirm Analyzer#hits can read a value from a TIMDEX response (or a mocked response), so we aren't always just testing with stubs. We can stub, but only if we also test the real method.
There was a problem hiding this comment.
Yeah, I can see a need for something like this. Do you want to add it to this PR, or make a separate ticket for it?
(Having looked at all of this changeset, I'm starting to think that introducing mocha is pretty close to being worthy of its own PR - but the change is already here so I don't want to ask that it be split apart now)
There was a problem hiding this comment.
I agree landing mocha like this wasn't ideal, but those tests were a nightmare to maintain and even with the note explaining the problem in the test I wasn't able to figure out easily how to "fix" the test with the new data in the cassette. I'm not opposed to moving it, but the result would be for me to set that test to "skip" for now and introduce mocha immediately after this unskipping it. I will not fix that broken test the old way one more time :)
There was a problem hiding this comment.
I've opened https://mitlibraries.atlassian.net/browse/RDI-215 to more meaningfully test Analyzer#hits
There was a problem hiding this comment.
Most of this looks good, I think. There's one thing I think is still needed (although maybe I've missed it) - a test of the timdex record query to confirm that it correctly responds to the TIMDEX_INDEX env variable. We have such a test for the search side, but nothing that I can see for the record.
My other question is non-blocking on this PR, as I think it raises bigger questions than this ticket.
I wanted to ask about is whether the UI itself needs to change in response to the ENV being set. Right now the only indication to the user that the app is searching a non-standard index is the results being displayed. That worries me a little, as the user may not notice which URL they're using or not appreciate the implications of the active environment.
We could communicate this via the alert banner, but that would need to be kept in sync with the env (as well as the env array of available sources). It might make more sense to either add some UI feature above the search box indicating a non-standard index, or populate the list of search sources from the set of available indexes (which implicates a rather large set of changes to both this application and the API).
Again, this point is non-blocking on the PR merging. The only blocking request is for a test in the timdex record area to make sure that the passed index is responsive of the set env variable.
| @@ -56,4 +56,16 @@ class QueryBuilderTest < ActiveSupport::TestCase | |||
| } | |||
| assert_equal(expected, QueryBuilder.new(search).query) | |||
| end | |||
|
|
|||
| test 'query builder can read TIMDEX_INDEX from env' do | |||
There was a problem hiding this comment.
I see this test for the QueryBuilder, which takes care of this functionality for searches. I don't see something similar on the record side, though?
The changes to record_controller_test.rb are renaming cassettes (which is a good thing to do, we've talked about cassette names needing a bit more structure).
Looking at the change to the controller itself, I also don't know that a test makes sense there. I can't think of what a meaningful controller test would be, as the change is just to load a value from env (which feels like the sort of thing that we shouldn't have to test).
These tests for the query builder make sure that the built query reflects what it is told - so I think the test I'm looking for would be something testing /app/models/timdex_record.rb to make sure that the query it builds is responsive to changed env.
Maybe something under https://github.com/MITLibraries/timdex-ui/blob/main/test/models/timdex_test.rb#L17 ?
There was a problem hiding this comment.
Part of the problem is the existing test suite is a bit of a mess and needs a refactor. I'll add the tests you are looking for somewhere and when we discuss testing in general as a team we can refactor. I don't want to introduce a significant change to testing in this changeset (as you noted elsewhere, I already snuck in a refactor of some unrelated tests to move them to stubs which ideally should have been done separately).
|
@matt-bernhardt do you think this starts to address your concern about the different UI versions for that each have different data? Ultimately, each "site" we deploy will need to address what is being searched for sure. I do agree with you that while this is a big concern, and I think if we end up with multiple production instances, we'll definitely need to work with UX to ensure users are not confused as to what they are searching. edit: forgot to add the link: https://mitlibraries.atlassian.net/browse/RDI-213 |
|
Yeah, that ticket captures most of what I was thinking of. The thing it doesn't address, which is a very different piece of engineering, would be something like an API endpoint similar to the output of For now, the ticket you've made is fine IMO. |
There was a problem hiding this comment.
After talking through the issue with the test that I was asking for, I now agree that this isn't something that we can do now - and further changes to the code in this PR are probably a bad idea to keep things manageable.
I'm switching my review to 
- I look forward to talking through how to improve our testing approach, part of which will be how to most appropriately test for the feature that I was looking for.
Thanks for taking the time to talk this through with me this afternoon - and for doing the work for these two PRs.
Why these changes are being introduced: The 'view online' links in the result view don't currently go anywhere. Relevant ticket(s): https://mitlibraries.atlassian.net/browse/RDI-238 How this addresses that need: This assigns the 'view online' hrefs to the sourceLink of each record. RDI records will always use sourceLink for this purpose, so this approach is sufficient for the time being. Side effects of this change: * Future non-RDI use cases may require us to update this logic. * Due to the schema change, all cassettes have been regenerated. * TIMDEX_INDEX has been added to the test environment, as per #63. Additionally, some tests have been updated to accommodate data returned by the RDI index. Update schema, regenerate cassettes, and add TIMDEX_INDEX to test env
NOTE: the PR build is configured to use the PR build for the associated TIMDEX change MITLibraries/timdex#540 as well as having the
TIMDEX_INDEXset tordi*Why are these changes being introduced:
provide an RDI specific instance of this app
Relevant ticket(s):
How does this address that need:
of the index
TIMDEX_INDEXvalue in GraphQL query if it is set in ENV, ifit is not set, the TIMDEX endpoint will default to whatever default
value it is configured for
Side effect:
tests to avoid having to use cassettes to test the Analyzer. This
removes a fragile test.
Record and they now need their own cassettes because the GraphQL is slightly different (we also could have updated the model tests but
this seemed easier and less fragile down the road to not couple unrelated tests to the same cassette)
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO