Skip to content
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

receivers: implement autocomplete #3006

Closed
wants to merge 20 commits into from

Conversation

iulianav
Copy link

@iulianav iulianav commented Nov 29, 2017

Description

Implemented the back-end for autocompletion using ES completion type suggesters for several required fields.

Related Issue

inspirehep/record-editor#261
closes #2755

Motivation and Context

Needed for the front-end implementation of the autocompletion..

Checklist:

  • I have all the information that I need (if not, move to RFC and look for it).
  • I linked the related issue(s) in the corresponding commit logs.
  • I wrote good commit log messages.
  • My code follows the code style of this project.
  • I've added any new docs if API/utils methods were added.
  • I have updated the existing documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@@ -281,6 +281,9 @@
},
"volume": {
"type": "string"
},
"book_series_suggest": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since, this field is inside book_series, adding the prefix book_series is duplicating information.
I would suggest having that solely as suggest.

@@ -310,6 +313,9 @@
},
"value": {
"type": "string"
},
"collaboration_suggest": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same goes for this field.

@@ -292,7 +354,8 @@ def populate_abstract_source_suggest(sender, json, *args, **kwargs):


def populate_title_suggest(sender, json, *args, **kwargs):
"""Populate the ``title_suggest`` field of Journals records."""
"""Populate the ``title_suggest`` field of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: formatting

@iulianav iulianav force-pushed the implement-autocomplete branch 2 times, most recently from 5ab546a to 2f8863a Compare December 1, 2017 10:09
@@ -79,6 +79,10 @@
"conferenceautocomplete": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this still be used after this PR (I have no idea)? if not you should remove it.

@@ -132,6 +132,10 @@
"experimentautocomplete": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment.

@@ -148,7 +153,7 @@ def enhance_after_index(sender, json, *args, **kwargs):


def populate_bookautocomplete(sender, json, *args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so what's the difference between *autocomplete and *_suggest? if there is none, the names should probably made consistent one way or another.

Copy link
Author

@iulianav iulianav Dec 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michamos we will stick to the *_suggest as this is the proper way of implementing the suggesters. We will also remove all *autocomplete suggesters.

return

doc_types = json.get('document_type', [])
if all(doc_type not in doc_types for doc_type in ['book', 'thesis', 'proceedings']):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find if not set(doc_types) & {'book', 'thesis', 'proceedings'} clearer.

'title': titles,
},
},
})


def populate_book_series_suggest(sender, json, *args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there seems to be a book series autocomplete in the submission forms. It is broken however, as it autocompletes from all journals, instead of only those having book_series=True combined with the book series in Literature records. Can you check whether you can salvage that one instead of creating a new one?

Copy link
Author

@iulianav iulianav Dec 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michamos I would say that we should probably try to use this one in oreder to have consistent suggesters and remove the old one. @jacquerie, @harunurhan ? Also, the old one seems overly-complicated and confusing I would say.

'title': titles,
},
},
})


def populate_book_series_suggest(sender, json, *args, **kwargs):
"""Populate the ``book_series.book_series_suggest`` field of Literature records."""
if 'hep.json' not in json.get('$schema'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this really one should check for a given record type (cc @jacquerie)? I would expect there to be a helper somewhere that gets you the type from the record. If not, it should probably exist.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michamos is this related: #1355 ?


json.update({
'conference_suggest': {
'input': cnum,
Copy link
Contributor

@michamos michamos Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autocompleting on cnum is not very useful: either you already know it and you can type it out, or you don't and you won't learn anything from the list of suggestions. You should add more information from the conference record:
acronyms, address, series.name, titles, opening_date.


input_values = []
input_values.append(legacy_name)
input_values.append(long_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather do input_values = [legacy_name, long_name]

@@ -306,14 +417,17 @@ def populate_title_suggest(sender, json, *args, **kwargs):
input_values.extend(title_variants)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same story

@michamos
Copy link
Contributor

michamos commented Dec 7, 2017

This is probably relevant: https://github.com/inveniosoftware/invenio/wiki/Invenio-Sprint-Reviews#completion-suggesters

@iulianav
Copy link
Author

iulianav commented Dec 7, 2017

@michamos should we change to the V5 way of doing things that uses _source?

@michamos
Copy link
Contributor

michamos commented Dec 7, 2017

not my decision :) @jacquerie ?

@harunurhan
Copy link
Contributor

harunurhan commented Dec 8, 2017

If you remove the payload, it will be a breaking change. For record-editor, it will be small and quick PR to fix it, no problem but I don't know much about other parts.

@iulianav iulianav force-pushed the implement-autocomplete branch 2 times, most recently from bda7ccf to fb97a92 Compare December 8, 2017 13:47
@@ -170,20 +175,106 @@ def populate_bookautocomplete(sender, json, *args, **kwargs):
input_values.extend(titles)
input_values = [el for el in input_values if el]

ref = get_value(json, 'self.$ref')
record = get_value(json, 'self.$ref', '')

json.update({
'bookautocomplete': {
'input': input_values,
'payload': {
'authors': authors,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michamos I need more indications for bookautocomplete. Currently it has no output and we need to decide on it and what should be in the payload, right @harunurhan ?

Copy link
Contributor

@michamos michamos Dec 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

input seems fine, output for parent_isbn should be isbns.value[0].

@iulianav
Copy link
Author

iulianav commented Dec 8, 2017

Includes also and closes: #2795

@iulianav iulianav force-pushed the implement-autocomplete branch 3 times, most recently from e7d7b33 to 32460aa Compare December 11, 2017 12:13
@iulianav
Copy link
Author

Includes and closes #2837

@jacquerie
Copy link
Contributor

jacquerie commented Dec 11, 2017

Includes and closes #2837

This doesn't work: annotations like "(c|C)loses #XXX" only take effect if they are part of the first message of the PR (#3006 (comment)) or as part of any commit. See: https://help.github.com/articles/closing-issues-using-keywords/.

Iuliana Voinea added 18 commits January 31, 2018 10:29
Added unit tests for hep records suggesters: collaboration_suggest and
bokk_series_suggest, and for experiments records experiment_suggest suggester.

Signed-off-by: Iuliana Voinea <[email protected]>
Added 'long_name' and 'name_variants' to the input of experiment_suggest.

Signed-off-by: Iuliana Voinea <[email protected]>
So far, even if a record has several report numbers associated with it,
the output will contain only the first one.

Signed-off-by: Iuliana Voinea <[email protected]>
The autocompletion is now done using a completion suggester. `experiment_suggest`.

Signed-off-by: Iuliana Voinea <[email protected]>
I replaced the old `conferenceautocomplete` with `conference_suggest`. Also,
there was a bug in the JavaScript which called the `conference_suggest` endpoint
every time the submission form was laoded, which I also hopefully fixed.

Lastly, I removed some commented/unused autocompletion functions from the forms
module and added `TODO`. comments which mention the fact that we need a consistent
way of performing autocompletion (with ES compeltion suggeters), instead of using
the old way this was implemented in `INSPIREForm`.

Signed-off-by: Iuliana Voinea <[email protected]>
Signed-off-by: Iuliana Voinea <[email protected]>
@iulianav
Copy link
Author

iulianav commented Jan 31, 2018

Some acceptance tests for conference and advisors autocompletion were failing and I had to modify them. This is due to the fact that when trying to autocomplete for the conference IN2P3 School of Statistics, 2012-05-28, Autrans, FR for example, the test tries to do so by typing autrans, but the way the ES completion suggester works does not allow for this as we would need an arbitrary offset, but I do not think this is possible to configure: The completion suggester is a so-called PREFIX suggester. It would have to type IN2 in order to work. Also, the tests expected an output that is no longer accurate.

However, for an author Smith, John the tests might pass for both typing Smi or Joh ( note that the required minimum length for autocompletion is 3 characters ) because there is a field called preferred_name which when populated seems to contain the value John Smith and is also added to the input_values needed for the suggester. Please someone correct me if I am wrong!

@iulianav iulianav force-pushed the implement-autocomplete branch 2 times, most recently from 7bb48e2 to 92ed6f0 Compare January 31, 2018 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in autocomplete API for Institutions
8 participants