-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to search library by old 5 digit library number #875
Comments
@seankwilliams I'm trying to duplicate this, and it seems to work okay in the dev environment. If I create a new album with a value for the Library Number field and then search that number, it will come up in the search results. I also tried entering a 5 digit number at random in the search bar and if there is an album with a corresponding number in the Library Number field, it will show. For instance I entered the number 53028 and it returned an album with that library number. Do you suppose this might only be happening on the live page? Let me know if I'm missing anything. |
@kmid5280 I just dug deeper on this and the problem seems specific to the search on Show Builder. The search on the music library is properly returning the album based on library number search. Can you compare what's different between those two and see if the Show Builder search can search by library number as well? |
@seankwilliams Do you suppose this might be happening because the Library Search Page is searching through all available information, whereas the Show Builder search is only searching through the track information (which would cause it to skip over the album information?) |
@kmid5280 yes, that's a great thought and that very well could be what's happening here. If that is the case, you'd have to modify the Mongo query on the back-end so it also considers searching the track's related album's library number field -- which is easy to say, but kind of a challenging query to write with how Mongo makes you query related entities. |
@seankwilliams What about if we had the search function the same way as the one on the Library Search page, but have it filter the tracks only? |
@kmid5280 That could work. I believe the Show Builder search has some logic that combines the search results from the library with search results from the iTunes API, which includes some tracks that are not entered into Comrad. If I recall correctly, combining the iTunes API search logic with the same way the library search page searches is probably an easier route than querying library number with the existing query. |
@kmid5280 just to help on some of the logic, the show builder is calling the search API at: client\src\pages\ShowBuilderPage\ShowBuilderPage.js line 490: Which in turn calls the search in the file at client\src\redux\library\actions\search.js Which in turn calls the API on the back-end at The challenge with searching everything and filtering down to tracks is that the search results will have to respect the limit passed into the search API, which I believe is 30 results. If I recall correctly, the API limits the responses to 30 results but fill sin the remainder with additional results from the iTunes API if fewer than 30 results are in the Comrad database. |
@seankwilliams If I add the following code to server\v1\controllers\library\search.js in the "track" switch case (around line 233 on my end), it will trigger correctly and log the old library number in the console if present:
I'm wondering if there's a place in here we can put a conditional that will include that track in the results if the track happens to contain one of those numbers? Or is this the right file to be attempting that in? |
@kmid5280 By that point in the code, the results have already been returned from Mongo. This is the code that does the library search:
I believe that code is doing a search on a
Then, see if you can find how Comrad updates the Here's the note on that field from the library model. Seems like a good place to add the custom library number for a track:
|
@seankwilliams I'm not finding anywhere in the code that governs which fields are searched depending on the type that is being searched, i.e. whether you are searching for an album, an artist, or a track. If I search for a library number that I know exists, it will return the correct results on the Library Search Page if I set the filter to Albums, but not if I set the filter to Artists or Tracks. At the same time, there doesn't seem to be any conditional anywhere that specifically instructs the find method to search for the library_number field, in any case. I do see there is a generateSearchIndexForLibrary and an updateSearchIndex page, but I'm not really sure where these run or what triggers them. They won't generate console logs when I start the app or run searches. They also do not reference specific searchable fields. Do you suppose the issue might be that the library number is not included as as a property in the Schema for tracks? I do see that in the Library Schema, there is a |
@kmid5280 I'll write up some details on how the search index field works when I have a few minutes. Might take a few because of the holidays here, but I'll put some info together that I think will help with this. |
@kmid5280 At long last, here's some information on how the search index works: First, you'll see in If you check https://www.mongodb.com/docs/manual/reference/operator/query/text/, you'll see that the So, the next step is to check the model and see what fields are in the text index. Looking at That is putting the following fields in the text index:
If you look into those custom fields ( Instead, let's look at the //will include all fields that should be searched. this will bring related entities onto this field, like artist/album names This is exactly what we want to do, bring the related album's library number onto this field for a full text search. Next up, we'll need to see where
That first file is the key one we need to update. For tracks, we'll want to add the album's library number into the That second file is only used for initial seeding of the database. It's still good to update that, just so the search index will be correctly generated when devs seed a local database. |
@seankwilliams Thanks for this explainer. In |
@kmid5280 Yes, let's add a new use case for tracks and set it up as you describe! |
@seankwilliams I'm currently trying to understand how this works. Do you suppose if we write the first line of the 'track' case as the following, that it will fit the album library number into the search? It seems like, if tracks are being searched, we want it to search for albums and specify the custom library number. I'm having some trouble using console.log here to see if I'm identifying the library number correctly.
|
@kmid5280 Can you send the steps you are using to try to test the function? That might help with identifying why you aren't seeing results with console.log. As for this code:
What line are you thinking of adding it? This code would search the Library model for albums that match the library number you are specifying. |
@seankwilliams I was thinking of adding that to line 64, just creating a new case for tracks. I assume that after I add this new line, I'll have to test it by updating one of the library objects? So after I add the new case, can I test it by creating a single new track and then searching for it via its associated album number? |
@kmid5280 If you're referring to adding that to line 64 of code in |
@seankwilliams Thanks. Do you suppose instead we need to change the parameters on line 35, to include the album library number there? |
@kmid5280 That's still part of the code that finds related entities to update: These are the three lines that save the updated search index: I'd go through that file line by line and see if you can learn what each line is doing. You'll want to understand mongoose and JavaScript promises to see what it's doing. |
@seankwilliams Do you suppose we'll have to make any changes to the |
@kmid5280 , yep, that is correct! The |
@seankwilliams So what about a conditional like the following, starting at line 219:
I see that the album property is used by tracks, so I'm telling it to check if there's an album, as well as a custom library number. Since we want to add the album to the list, that's why we would be adding the album itself via findById. Does it sound like this is on the right track to you? |
@kmid5280 That's getting close. There's two things you want to look at though:
|
@seankwilliams If I go to the Show Builder page, search for a track, console.log the result, and look at the object with the track data, I see that it has a So what if we update the search parameters to the following? If we are searching by tracks, and the track will show up if it has an associated album and
|
@kmid5280 I think you're missing a few pieces on how the search is working as a whole and the purpose of
|
@seankwilliams Thanks for the comment. I'll go back and review some of this material then. Couple questions: in |
@kmid5280 Good questions! Here's the declarations of each of those: Since That doesn't help still, because This does the following:
libraryLookupsTo understand this one, you've got to grasp asynchronous programming with JavaScript. I'm frankly weak at that - I find it hard to read and don't entirely understand the reasoning behind why Node.JS relies so heavily on asynchronous JavaScript. But to write Node you've got to have a handle on asynchronous JS otherwise it's way too easy to get lost. So, if you aren't up to speed with promises and async functions, I'd look up some resources on that and read up on it.
Finally, we get to this part of the code:
That says "wait until all of the promises we started are done". Then, the results of each promise is returned into an array of |
@seankwilliams I'm going back over some of the previous comments. Am I correct that this is structured so that when you update an item in the library (say an album, track, etc.) by editing it, this also triggers an update to the search index afterwards via a promise so that it will show up in a search? I want to conceptually understand where we are updating the item's content versus updating the search index. |
@kmid5280 yep, that is absolutely correct! |
@seankwilliams Tell me if this sounds right - ideally, if we update an album, and the album contains a library number, this should trigger the search index update so that that library number is then included in the index moving forward. Then, it will show up whenever we search for tracks too. Therefore we don't have to include any specific logic for tracks in |
@kmid5280 yes, that sounds like that's on the right track! Without digging deeper I am not sure if you'd need to add the library number to both the search index for tracks and the album, or just for one or the other. The track might need the change - it depends on which library type the search is looking at. |
@seankwilliams Going back to your earlier comment, how do you test a database record to see if it contains the fields I want the index to update? Can I just console.log a search result and verify that the fields are in there that way? |
@kmid5280 Yep, that's one way to do it. The other ways are to (1) download and use MongoDB Compass, which is a UX around Mongo DB, or (2) connect to Mongo via command line and query for the record you want to check out. |
@seankwilliams Below is a console.log of a track result from Show Builder that shows an So what if we make the conditional in
|
@kmid5280 This is on the right track. I would check to be sure that If In the code you've got, this part of the code is supposed to look up the full album object based on |
@seankwilliams Are you saying that the conditional |
@kmid5280 It depends on the database query used to retrieve Here's a query out of MongoDB Compass for library records that have an As you can see, So, on the Node.js side, The code in getSearchIndexForLibrary in the models/library.js file is assuming that the related entities like |
@seankwilliams When you say to look in the response of a
|
@kmid5280 Let me ask you this: can you explain why |
@seankwilliams If I understand right, |
@kmid5280 Your explanation actually relates to the code in As far as the library lookups code in
But why couldn't we just do this, which would be much simpler code-wise?
|
@seankwilliams I think I'm making some progress on this. First to answer your question, we need to have (This confused me at first because I thought I was able to get the library number added to the track's search index by rewriting the following function. There are two things I'm running into with this. One, this could probably be consolidated without having to run
|
@kmid5280 Very good work, this is spot on! Regarding this one: One, this could probably be consolidated without having to run findById twice, though I'm not sure how to return two separate values with one return statement. Yep, you're correct. The search index is saved to the database as a list of words separated by spaces, so you can do something like this:
As for the Show Builder search, I'm not sure what's going on there. Can you push your branch and I'll test it locally on my end to see? |
@seankwilliams I verified that your version of the code is returning the correct values and adding them to the search index, so I went ahead and went with that. Pushed it and made a PR. I'm testing it by going to the Show Builder and searching for a track via its album library number. Currently it's not showing up so wondering if there's another step in this? |
@seankwilliams Sorry, not sure if the PR was necessary at this stage, I can close it if not. |
Thanks @kmid5280 ! Making a PR is fine, I'll wait to merge it until we're sure it's working. I'll test that out! Hopefully soon, but it may take me a few days. |
Change the library search so, in addition to the current fields it looks at, it will also consider the value at custom.library_number
The text was updated successfully, but these errors were encountered: