|
Description
Jonathan Druart
2017-03-08 18:57:38 UTC
Created attachment 60931 [details] [review] Bug 18235: DBIx::Class schema changes Created attachment 60932 [details] [review] Bug 18235: DB changes Created attachment 60933 [details] [review] Bug 18235: ES - Facets configurable This patch adds a new section 'Facet order' in the Biblio tab of the 'Search engine configuration' admin page of the Elastic mappings. The idea is to let the librarians define the facet to display and order them as their needs. The ergonomic is not perfect and I am open to any suggestions. Test plan: Move up and down the field list to order the facets Hide/show some facets At the OPAC and the staff interface you should see the changes on the search result page. Created attachment 61528 [details] [review] Bug 18235: DBIx::Class schema changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 61529 [details] [review] Bug 18235: DB changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 61530 [details] [review] Bug 18235: ES - Facets configurable This patch adds a new section 'Facet order' in the Biblio tab of the 'Search engine configuration' admin page of the Elastic mappings. The idea is to let the librarians define the facet to display and order them as their needs. The ergonomic is not perfect and I am open to any suggestions. Test plan: Move up and down the field list to order the facets Hide/show some facets At the OPAC and the staff interface you should see the changes on the search result page. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> I like this alot - many libraries want to control the facets a bit more and this is helpful. Still a few tings that would be nice (but I am not suggesting they are needed in this patchset) -labels shouldn't be hardcoded - staff shoul eb able to set them -we should add more options to what can be faceted- could be out of control but technically any field should be able to be used -ES has better handling for faceting (can provide subsets of existing/current results and allow to step in/out of facets) FYI not complaints Nick, yes you are right of course. This is only the first step. However there are two problems: 1. Translatability: if staff changes or add new labels, they will not be translated (we could cover some in the template) 2. Shared includes files: What I said for 2 years now... We are using the same files for elastic and Zebra, so we have to stick to ugly structure the template expects. That's very bad, we need to create separate files to allow more flexibility. Marcel, They apply correctly, just skip the first patch and regenerate the schema files. If I select say Author and set them all to Facetable No in the Search fields list on the Biblios tab, I would not expect Author to be still in the Facet order list? Changing status for feedback Created attachment 64747 [details] [review] Bug 18235: DBIx schema changes Fresh changes for SearchField, facet_order. (In reply to Marcel de Rooy from comment #10) > If I select say Author and set them all to Facetable No in the Search fields > list on the Biblios tab, I would not expect Author to be still in the Facet > order list? I agree, this is not ideal, but I think that comes when we stop hardcoding the displayable facets all together - I would say beyond the scope of this bug Yes indeed you are right, this is not ideal and might be confusing. We should add more JS code to handle that correctly, but I am not willing to write it. However I am still thinking that this feature is needed. Also patch does not apply. Created attachment 69485 [details] [review] Bug 18235: DB changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 69486 [details] [review] Bug 18235: ES - Facets configurable This patch adds a new section 'Facet order' in the Biblio tab of the 'Search engine configuration' admin page of the Elastic mappings. The idea is to let the librarians define the facet to display and order them as their needs. The ergonomic is not perfect and I am open to any suggestions. Test plan: Move up and down the field list to order the facets Hide/show some facets At the OPAC and the staff interface you should see the changes on the search result page. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 69487 [details] [review] Bug 18235: DBIx schema changes Fresh changes for SearchField, facet_order. Created attachment 72936 [details] [review] Bug 18235: DB changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Current status: Signed Off Created attachment 72937 [details] [review] Bug 18235: ES - Facets configurable This patch adds a new section 'Facet order' in the Biblio tab of the 'Search engine configuration' admin page of the Elastic mappings. The idea is to let the librarians define the facet to display and order them as their needs. The ergonomic is not perfect and I am open to any suggestions. Test plan: Move up and down the field list to order the facets Hide/show some facets At the OPAC and the staff interface you should see the changes on the search result page. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 72938 [details] [review] Bug 18235: DBIx schema changes Fresh changes for SearchField, facet_order. Created attachment 72999 [details] [review] Bug 18235: DB changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Created attachment 73000 [details] [review] Bug 18235: ES - Facets configurable This patch adds a new section 'Facet order' in the Biblio tab of the 'Search engine configuration' admin page of the Elastic mappings. The idea is to let the librarians define the facet to display and order them as their needs. The ergonomic is not perfect and I am open to any suggestions. Test plan: Move up and down the field list to order the facets Hide/show some facets Rebuild index At the OPAC and the staff interface you should see the changes on the search result page. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Created attachment 73001 [details] [review] Bug 18235: DBIx schema changes Fresh changes for SearchField, facet_order. Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Added missing curly, removed a <<<<<HEAD thingy, added index rebuild in test plan. Ergonomic suggestion: - move the the facet table in a new tab (discussed with some interested libraries). Currently it is not visible at the first glance. QA suggestion: - rename bug_xxxxx.sql with the appropriate bug number, - add unit test for get_facetable_fields Then, nice feature and i would like this to move forward. +1 for the new 'Facet order' tab ! I did not create a new tab because we already have 3:
Search fields
Biblios
Authorities
and I do not think it makes sense to have a 4th "Facet order" which will refer to biblio search only.
Created attachment 73193 [details] [review] Bug 18235 - Add unit tests for get_facetable_fields rename atomic update file QA: Looking here There is a lot of noise from YAML while running qa tools and a git manipulation fail too. (In reply to Marcel de Rooy from comment #30) > There is a lot of noise from YAML while running qa tools and a git > manipulation fail too. Noise appears before and after the patch, so not related. The git manipulation is on the atomic update (it should not have been moved in the last patch), I will fix it on pushing. Resuming here * Commit title does not start with 'Bug XXXXX: ' - 9400c92 POD is missing for get_facetable_fields (In reply to Marcel de Rooy from comment #33) > * Commit title does not start with 'Bug XXXXX: ' - 9400c92 > POD is missing for get_facetable_fields That's all? Created attachment 74642 [details] [review] Bug 18235: Add unit tests for get_facetable_fields rename atomic update file Created attachment 74643 [details] [review] Bug 18235: Add missing POD to get_facetable_fields (In reply to Jonathan Druart from comment #35) > Created attachment 74642 [details] [review] [review] > Bug 18235: Add unit tests for get_facetable_fields > > rename atomic update file Looks like this patch does not what it was named for. Atomic update file's name is still Bug_xxxxxxx. Created attachment 75995 [details] [review] Bug 18235: DB changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Created attachment 75996 [details] [review] Bug 18235: Add unit tests for get_facetable_fields I have amended the last 2 patches: 1. rename atomic update file and 2. remove the line from commit message that was not related to the patch. This patch doesn't apply right now. Might not be a hard fix if you are familiar with the Elasticsearch code. Can we get a rebase please? @Ere - It'd also be great if you could have a look here. Created attachment 85212 [details] [review] Bug 18235: ES - Facets configurable This patch adds a new section 'Facet order' in the Biblio tab of the 'Search engine configuration' admin page of the Elastic mappings. The idea is to let the librarians define the facet to display and order them as their needs. The ergonomic is not perfect and I am open to any suggestions. Test plan: Move up and down the field list to order the facets Hide/show some facets Rebuild index At the OPAC and the staff interface you should see the changes on the search result page. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Created attachment 85213 [details] [review] Bug 18235: Add missing POD to get_facetable_fields Created attachment 85214 [details] [review] Bug 18235: DB changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Created attachment 85215 [details] [review] Bug 18235: Add unit tests for get_facetable_fields Created attachment 85216 [details] [review] Bug 18235: DBIx schema changes Fresh changes for SearchField, facet_order. Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Those patches have been waiting for one year, only for a commit message problem. As I do not plan to rebase them endlessly, I think it would be great for libraries using ES to get a green stamp from people involved in the ES development. Created attachment 85885 [details] [review] Bug 18235: (QA follow-up) Fix POD Created attachment 85886 [details] [review] Bug 18235: (QA follow-up) Add missing filters Comment on attachment 85214 [details] [review] Bug 18235: DB changes Review of attachment 85214 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/atomicupdate/bug_18235-add-search_field-facet_order.sql @@ +1,1 @@ > +ALTER TABLE search_field ADD COLUMN facet_order TINYINT(4) DEFAULT '0' AFTER weight; Here is default 0 ::: installer/data/mysql/kohastructure.sql @@ +1493,4 @@ > `label` varchar(255) NOT NULL COMMENT 'the human readable name of the field, for display', > `type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum', 'isbn', 'stdno') NOT NULL COMMENT 'what type of data this holds, relevant when storing it in the search engine', > `weight` decimal(5,2) DEFAULT NULL, > + `facet_order` TINYINT(4) DEFAULT NULL COMMENT 'the order place of the field in facet list if faceted', And here NULL Comment on attachment 85212 [details] [review] Bug 18235: ES - Facets configurable Review of attachment 85212 [details] [review]: ----------------------------------------------------------------- ::: Koha/SearchEngine/Elasticsearch.pm @@ +971,5 @@ > + my @faceted_fields = Koha::SearchFields->search( > + { name => { -in => \@search_field_names }, facet_order => { '!=' => undef } }, { order_by => ['facet_order'] } > + ); > + my @not_faceted_fields = Koha::SearchFields->search( > + { name => { -in => \@search_field_names }, facet_order => undef }, { order_by => ['facet_order'] } Why do you retrieve fields from database in 2 queries, you always use them all. ::: admin/searchengine/elasticsearch/mappings.pl @@ +215,4 @@ > push @all_search_fields, $search_field_unblessed; > } > > +my @facetable_fields = Koha::SearchEngine::Elasticsearch->get_facetable_fields(); get_facetable_fields is called 3 or 4 times every run of mappings.pl. Hi Jonathan, great job, I have just some small questions (see above) I also adding Owen if he can advice us how to do the UI better, it is not obvious that the facet order could be changed, also the facet table is at the end of page and easy to oversee... (In reply to Josef Moravec from comment #52) > the facet table is at > the end of page and easy to oversee... I wonder if it should be in a separate tab? I would support an integration ASAP then we could discuss the ergonomic (patches submitted 2 years ago). Unless somebody else is willing to make the changes ;) I've got some interface changes in process, but they don't need to hold this up. (In reply to Josef Moravec from comment #50) > Comment on attachment 85214 [details] [review] [review] > Bug 18235: DB changes > > Review of attachment 85214 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: > installer/data/mysql/atomicupdate/bug_18235-add-search_field-facet_order.sql > @@ +1,1 @@ > > +ALTER TABLE search_field ADD COLUMN facet_order TINYINT(4) DEFAULT '0' AFTER weight; > > Here is default 0 > > ::: installer/data/mysql/kohastructure.sql > @@ +1493,4 @@ > > `label` varchar(255) NOT NULL COMMENT 'the human readable name of the field, for display', > > `type` ENUM('', 'string', 'date', 'number', 'boolean', 'sum', 'isbn', 'stdno') NOT NULL COMMENT 'what type of data this holds, relevant when storing it in the search engine', > > `weight` decimal(5,2) DEFAULT NULL, > > + `facet_order` TINYINT(4) DEFAULT NULL COMMENT 'the order place of the field in facet list if faceted', > > And here NULL Right, must be NULL in the atomic update file. (In reply to Josef Moravec from comment #51) > Comment on attachment 85212 [details] [review] [review] > Bug 18235: ES - Facets configurable > > Review of attachment 85212 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/SearchEngine/Elasticsearch.pm > @@ +971,5 @@ > > + my @faceted_fields = Koha::SearchFields->search( > > + { name => { -in => \@search_field_names }, facet_order => { '!=' => undef } }, { order_by => ['facet_order'] } > > + ); > > + my @not_faceted_fields = Koha::SearchFields->search( > > + { name => { -in => \@search_field_names }, facet_order => undef }, { order_by => ['facet_order'] } > > Why do you retrieve fields from database in 2 queries, you always use them > all. To have the not faceted fields at the end of the array. I agree it's a lazy way to handle that, but it works :) > ::: admin/searchengine/elasticsearch/mappings.pl > @@ +215,4 @@ > > push @all_search_fields, $search_field_unblessed; > > } > > > > +my @facetable_fields = Koha::SearchEngine::Elasticsearch->get_facetable_fields(); > > get_facetable_fields is called 3 or 4 times every run of mappings.pl. Yep, could be lowered to 2. Created attachment 85969 [details] [review] Bug 18235: Update facet_order default value in atomic update Created attachment 85970 [details] [review] Bug 18235: Do not call get_facetable_fields excessively (In reply to Owen Leonard from comment #53) > (In reply to Josef Moravec from comment #52) > > the facet table is at > > the end of page and easy to oversee... > > I wonder if it should be in a separate tab? I think it is good idea. (In reply to Jonathan Druart from comment #54) > I would support an integration ASAP then we could discuss the ergonomic > (patches submitted 2 years ago). > > Unless somebody else is willing to make the changes ;) (In reply to Owen Leonard from comment #55) > I've got some interface changes in process, but they don't need to hold this > up. I agree, we could pass and push this soon and then work on UI Created attachment 85983 [details] [review] Bug 18235: ES - Facets configurable This patch adds a new section 'Facet order' in the Biblio tab of the 'Search engine configuration' admin page of the Elastic mappings. The idea is to let the librarians define the facet to display and order them as their needs. The ergonomic is not perfect and I am open to any suggestions. Test plan: Move up and down the field list to order the facets Hide/show some facets Rebuild index At the OPAC and the staff interface you should see the changes on the search result page. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85984 [details] [review] Bug 18235: Add missing POD to get_facetable_fields Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85985 [details] [review] Bug 18235: DB changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85986 [details] [review] Bug 18235: Add unit tests for get_facetable_fields Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85987 [details] [review] Bug 18235: DBIx schema changes Fresh changes for SearchField, facet_order. Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85988 [details] [review] Bug 18235: (QA follow-up) Fix POD Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85989 [details] [review] Bug 18235: (QA follow-up) Add missing filters Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85990 [details] [review] Bug 18235: Update facet_order default value in atomic update Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85991 [details] [review] Bug 18235: Do not call get_facetable_fields excessively Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 85992 [details] [review] Bug 18235: (QA follow-up) Make db update idempotent Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Conflict in the mappings file, please rebase Created attachment 86340 [details] [review] Bug 18235: ES - Facets configurable This patch adds a new section 'Facet order' in the Biblio tab of the 'Search engine configuration' admin page of the Elastic mappings. The idea is to let the librarians define the facet to display and order them as their needs. The ergonomic is not perfect and I am open to any suggestions. Test plan: Move up and down the field list to order the facets Hide/show some facets Rebuild index At the OPAC and the staff interface you should see the changes on the search result page. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86341 [details] [review] Bug 18235: Add missing POD to get_facetable_fields Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86342 [details] [review] Bug 18235: DB changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86343 [details] [review] Bug 18235: Add unit tests for get_facetable_fields Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86344 [details] [review] Bug 18235: DBIx schema changes Fresh changes for SearchField, facet_order. Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86345 [details] [review] Bug 18235: (QA follow-up) Fix POD Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86346 [details] [review] Bug 18235: (QA follow-up) Add missing filters Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86347 [details] [review] Bug 18235: Update facet_order default value in atomic update Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86348 [details] [review] Bug 18235: Do not call get_facetable_fields excessively Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86349 [details] [review] Bug 18235: (QA follow-up) Make db update idempotent Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86350 [details] [review] Bug 18235: (QA follow-up) Series facet name after rebase Bug 19575 changes many field names so "se" facet field became "title-series" I rebased it and add a follow-up to adapt to 19575, just not sure what to do, with su-geo, look like it was removed in bug 1957... Bug 22495 is going to restore su-geo, so I think we should wait with this again :( Created attachment 86944 [details] [review] Bug 18235: ES - Facets configurable This patch adds a new section 'Facet order' in the Biblio tab of the 'Search engine configuration' admin page of the Elastic mappings. The idea is to let the librarians define the facet to display and order them as their needs. The ergonomic is not perfect and I am open to any suggestions. Test plan: Move up and down the field list to order the facets Hide/show some facets Rebuild index At the OPAC and the staff interface you should see the changes on the search result page. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86945 [details] [review] Bug 18235: Add missing POD to get_facetable_fields Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86946 [details] [review] Bug 18235: DB changes Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86947 [details] [review] Bug 18235: Add unit tests for get_facetable_fields Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86948 [details] [review] Bug 18235: DBIx schema changes Fresh changes for SearchField, facet_order. Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86949 [details] [review] Bug 18235: (QA follow-up) Fix POD Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86950 [details] [review] Bug 18235: (QA follow-up) Add missing filters Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86951 [details] [review] Bug 18235: Update facet_order default value in atomic update Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86952 [details] [review] Bug 18235: Do not call get_facetable_fields excessively Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86953 [details] [review] Bug 18235: (QA follow-up) Make db update idempotent Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86954 [details] [review] Bug 18235: (QA follow-up) Series facet name after rebase Bug 19575 changes many field names so "se" facet field became "title-series" Created attachment 86955 [details] [review] Bug 18253: (QA follow-up) Restore facet_order for su-geo field in mappings file Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 86956 [details] [review] Bug 18235: (QA follow-up) Restore facet_order for su-geo field in mappings file Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased and adapted to bug 22495. Created attachment 87141 [details] [review] Bug 18235: (RM follow-up) fix mappings reset Without this we could not recreate mappings as there are no defaults for the additional fields - this restores behavoiur from before patches Awesome work all! Pushed to master for 19.05 (In reply to Nick Clemens from comment #100) > Awesome work all! > > Pushed to master for 19.05 \o/ New feature will not be backported to 18.11.x series. |