-
Notifications
You must be signed in to change notification settings - Fork 21
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
Kiwix search implementation #676
base: main
Are you sure you want to change the base?
Conversation
backend/src/models/library.go
Outdated
type KiwixItem struct { | ||
Title string `json:"title"` | ||
Link string `json:"link"` | ||
Description string `json:"description"` | ||
Book string `json:"book"` | ||
ThumbnailUrl string `json:"thumbnail_url"` | ||
WordCount string `json:"word_count"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you alter this so this struct is deleted and anywhere you use this struct instead modify to return a Library?
@@ -55,6 +64,81 @@ func (srv *Server) handleGetLibrary(w http.ResponseWriter, r *http.Request, log | |||
return writeJsonResponse(w, http.StatusOK, library) | |||
} | |||
|
|||
func (srv *Server) handleSearchLibraries(w http.ResponseWriter, r *http.Request, log sLog) error { | |||
page, perPage := srv.getPaginationInfo(r) | |||
pattern := r.URL.Query().Get("pattern") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this search? to make this consistent with the terms used in the backend, could we make this query param be "search"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to use search instead of pattern
func (db *DB) GetLibraryOptions(viewScope string) ([]models.Option, error) { | ||
options := []models.Option{} | ||
tx := db.Model(models.Library{}).Select("libraries.id, libraries.title as label"). | ||
Joins(`JOIN open_content_providers p on p.id = open_content_provider_id | ||
AND p.currently_enabled = 'true'`) | ||
if viewScope != "all" { | ||
tx = tx.Where("visibility_status = true") | ||
} | ||
if err := tx.Order("libraries.title").Find(&options).Error; err != nil { | ||
return options, newGetRecordsDBError(err, "libraries") | ||
} | ||
return options, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than create a new function for this, wouldnt we be able to just call GetAllLibraries
? Its a messy function that probably (definitely) needs to be cleaned up, but should give you all the libraries you are searching for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would allow us to remove this function, and also the api "/api/libraries/options"
, and just directly call api/libraries
for all libraries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember that initially I was going to use the GetAllLibraries call, but because of the pagination logic (page, perPage) and the scalar sub select for is_favorites I shied away from it. I just want to confirm that I should move forward with making the GetAllLibraries work?
selectedLibraries := r.URL.Query().Get("libraries") | ||
splitedIDs := strings.Split(selectedLibraries, ",") | ||
libraryIDs := make([]int, 0, len(splitedIDs)) | ||
for _, id := range splitedIDs { | ||
if libID, err := strconv.Atoi(id); err == nil { | ||
libraryIDs = append(libraryIDs, libID) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of splitting this this way, you can pass in the library ids by mapping them into the query as separate calls (so, for example you would do api/..../...libraryId=1&libraryId=2&libraryId=3
etc) and then back here you would do r.URL.Query()["libraries"]
and then it will return it as an array, and you will have to map through that and convert to an array of integers. see example on user_handler.go
inside of handleIndexUsers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this functionality! :)
Description of the change
Initial design and implementation of doing full text searching across kiwix libraries. This change required deploying our own managed kiwix server within the 'staging' development environment so that we could manage Kiwix's API to perform full text searching.
Things to note
A Kiwix server was set up to provide control and customization over our search functionalities.
Libraries will need to be manually downloaded to the Kiwix server as part of the setup and maintenance process.
We need to ensure sufficient disk space is allocated on the server for storing library zim files--could grow significantly depending on the number of zim files downloaded.
Related issues: "Closes Task: Implement Kiwix Search #639 ".
Screenshot(s)
Additional context
No additional context.
If any core features or components were removed with this PR, please note them here so that they can be added to the wiki (see Deprecated features and Components).