Feature/3367 missing oaipmh records #2191
Conversation
…ema variable from settings and doaj js
amdomanska
requested review from
philipkcl,
richard-jones,
RK206 and
Steven-Eardley
portality/models/oaipmh.py
Outdated
| s["term"]["index.schema_subject.exact"] = oai_set | ||
| q["query"]["bool"]["must"].append(s) | ||
| a = oai_set.replace(constants.SUBJECTS_SCHEMA,"") | ||
| q["query"]["bool"]["should"] = {"match":{"index.classification": a}} |
There was a problem hiding this comment.
I think this should be on index.classification.exact ? Or does changing it from a term query to a match fix that?
Also, are we sure it should be a should query? Seems like is should be a must query like the one it is replacing?
There was a problem hiding this comment.
Yes, I'm confident that this should be a term query and a must.
Can we please lay it out the way way as it was before, so use self.set_limit and just modify it to use index.classification.exact instead.
There was a problem hiding this comment.
Yes, you're right. The should and match is the only way to make it work without the exact filter. When exact is applied, then it is must and term that works.
portality/models/oaipmh.py
Outdated
| s["term"]["index.schema_subject.exact"] = oai_set | ||
| q["query"]["bool"]["must"].append(s) | ||
| a = oai_set.replace(constants.SUBJECTS_SCHEMA,"") | ||
| q["query"]["bool"]["should"] = {"match":{"index.classification": a}} |
There was a problem hiding this comment.
Yes, I'm confident that this should be a term query and a must.
Can we please lay it out the way way as it was before, so use self.set_limit and just modify it to use index.classification.exact instead.
| @@ -71,8 +72,9 @@ def list_records(self, from_date=None, until_date=None, oai_set=None, list_size= | |||
| if start_after is not None or from_date is not None or until_date is not None or oai_set is not None: | |||
|
|
|||
| if oai_set is not None: | |||
| a = oai_set.replace(constants.SUBJECTS_SCHEMA,"") | |||
There was a problem hiding this comment.
variable a never used. I think is forgot to remove this line.
There was a problem hiding this comment.
@amdomanska can you confirm if this line should be used somewhere?
There was a problem hiding this comment.
This was a bug, that line is required, but the result of it was not used. I have fixed and pushed.
There was a problem hiding this comment.
@amdomanska there was one bug which I have fixed for the sake of getting this live sooner
| @@ -71,8 +72,9 @@ def list_records(self, from_date=None, until_date=None, oai_set=None, list_size= | |||
| if start_after is not None or from_date is not None or until_date is not None or oai_set is not None: | |||
|
|
|||
| if oai_set is not None: | |||
| a = oai_set.replace(constants.SUBJECTS_SCHEMA,"") | |||
There was a problem hiding this comment.
@amdomanska can you confirm if this line should be used somewhere?
| @@ -71,8 +72,9 @@ def list_records(self, from_date=None, until_date=None, oai_set=None, list_size= | |||
| if start_after is not None or from_date is not None or until_date is not None or oai_set is not None: | |||
|
|
|||
| if oai_set is not None: | |||
| a = oai_set.replace(constants.SUBJECTS_SCHEMA,"") | |||
There was a problem hiding this comment.
This was a bug, that line is required, but the result of it was not used. I have fixed and pushed.
"Missing OAIPMH records
Please don't delete any sections when completing this PR template; instead enter N/A for checkboxes or sections which are not applicable, unless otherwise stated below
See 3367
Describe the scope/purpose of the PR here in as much detail as you like
Categorisation
This PR...
Basic PR Checklist
constantsormessagesfilesdates)develop(or other base branch)Testing
Worth re-running the user's original request to check that they get the results they expect
Harvest the Architecture set: https://doaj.org/oai?verb=ListRecords&set=TENDOkFyY2hpdGVjdHVyZQ~~&metadataPrefix=oai_dc
Harvest the Fine Art set: https://doaj.org/oai?verb=ListRecords&set=TENDOkZpbmUgQXJ0cw~~&metadataPrefix=oai_dc
Look for "Urbanism. Arhitectura. Constructii".