Skip to content

MNT: Use new RTD search API #462

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

MNT: Use new RTD search API #462

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions tutorials/themes/tutorials-theme/static/searchtools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// See https://stackoverflow.com/a/25489055/9959073 for details.
jQuery.ajaxSetup({async:false});

/* Non-minified version JS is _stemmer.js if file is provided */
/* Non-minified version JS is _stemmer.js if file is provided */
/**
* Porter Stemmer
*/
Expand Down Expand Up @@ -416,17 +416,17 @@ var Search = {
if (query.includes("filterTutorials")) { // don't search docs if we are just looking for tutorials
var docResponse = null;
var docResult = '';
$.get('https://readthedocs.org/api/v2/docsearch/?q=' + window.location.search.split('=')[1] + '&project=astropy&version=stable&language=en', function(response){
$.get('https://docs.readthedocs.io/_/api/v2/search/?q=' + window.location.search.split('=')[1] + '&project=astropy&version=stable', function(response){
Copy link

Choose a reason for hiding this comment

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

The new endpoint should be https://docs.astropy.org/_/api/v2/search/?q=test&project=astropy&version=stable

Copy link

Choose a reason for hiding this comment

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

When I try to make this query from learn.astropy.org I get CORS errors:

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://docs.astropy.org/_/api/v2/search/?q=test&project=astropy&version=stable. (Reason: CORS header ‘Access-Control-Allow-Origin’ missing).

Is this related to readthedocs/readthedocs.org#6154 ? Is there any hope of working around this?

Copy link

Choose a reason for hiding this comment

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

oh, I think you can use the other endpoint https://readthedocs.org/api/v2/search/?q=test&project=astropy&version=stable let me know if that one works

Copy link

Choose a reason for hiding this comment

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

I still get

Access to XMLHttpRequest at 'https://readthedocs.org/api/v2/search/?q=test&project=astropy&version=stable' from origin 'http://learn.astropy.org' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

:/

Copy link

Choose a reason for hiding this comment

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

hmm, not sure how this was working before.

https://github.com/readthedocs/readthedocs.org/blob/12598254c86e954f19c7402395d9e402f9a2aaaa/readthedocs/core/signals.py#L33-L82

Looks like you can get around by creating a custom domain with "learn.astropy.org", there is no need to add the CNAME, this is so rtd knows this is a known domain.

Copy link

Choose a reason for hiding this comment

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

Oh, I found the settings on RTD for adding an additional custom domain. I see your point now about adding it as a custom domain without actually adding a CNAME entry. Hopefully that should work (it does not yet, but it still shows "SSL certificate status: pending_validation" on https://readthedocs.org/dashboard/astropy/domains/10700/edit/

Copy link

Choose a reason for hiding this comment

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

It appears to be due to this, but in this case RTD wouldn't be issuing an SSL cert on behalf of the site: https://docs.readthedocs.io/en/stable/custom_domains.html#certificate-authority-authorization

Copy link

Choose a reason for hiding this comment

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

Although looking again at the code you linked, for the CORS access it doesn't seem to care about certificate status :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

someone who was not me apparently made the decision at one point not to

@adrn , @eblur , or @kelle , do you remember why this decision was made? Can you please help @embray move this forward? Thanks.

Copy link

Choose a reason for hiding this comment

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

Although looking again at the code you linked, for the CORS access it doesn't seem to care about certificate status

Yeah, you only need to add the domain, the status or the CNAME don't matter here. Let me know if that fixed the CORS problem.

docResponse = response;
//limits the maximum results from the documentation to 15
docResultsCount = (docResponse.results.hits.total>15)? 15 : docResponse.results.hits.total ;
Copy link

Choose a reason for hiding this comment

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

This should be just results.length


for(var i = 0; i < docResultsCount; i++ ) {
docResult += '<li style="" class="result-card doc"> <a href="' +
docResponse.results.hits.hits[i].fields.link + '.html' +
docResult += '<li style="" class="result-card doc"> <a href="' +
docResponse.results.hits.hits[i].fields.link + '.html' +
Copy link

Choose a reason for hiding this comment

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

The link now includes the extension. results[i].domain + results[i].path

'?highlight=' + window.location.search.split('=')[1] + '">' +
docResponse.results.hits.hits[i].fields.title[0] + '</a>' +
Copy link

Choose a reason for hiding this comment

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

this should be results[i].title

'<div class="context">' +
'<div class="context">' +
docResponse.results.hits.hits[i].highlight.content[0] +
Copy link

Choose a reason for hiding this comment

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

This should be iterating over results[i].blocks

docResponse.results.hits.hits[i].highlight.content[1] +
'</div> </li>';
Expand Down Expand Up @@ -563,7 +563,7 @@ var Search = {
if (item[3]) {
listItem.append($('<span> (' + item[3] + ')</span>'));
Search.output.append(listItem);

listItem.slideDown(5, function() {
displayNextItem();
});
Expand All @@ -590,7 +590,7 @@ var Search = {
listItem.slideDown(5, function() {
displayNextItem();
});
}
}
}
// search finished, update title and status message
else {
Expand Down Expand Up @@ -775,19 +775,19 @@ var Search = {
*/
makeSearchSummary : function(text, keywords, hlwords) {
var textLower = text.toLowerCase();

//extracting an image for the thumbnail
var imageSearch = textLower.indexOf('nboutput',0);
var imageTypeJPEG = textLower.indexOf('.jpeg', imageSearch);
var imageTypePNG = textLower.indexOf('.png', imageSearch);
var imageTypeJPG = textLower.indexOf('.jpg', imageSearch);
imageType = ( (imageTypeJPEG == -1) || ((imageTypeJPG != -1) && (imageTypeJPG < imageTypeJPEG)) )? imageTypeJPG : imageTypeJPEG ;
imageType = ( (imageTypeJPEG == -1) || ((imageTypeJPG != -1) && (imageTypeJPG < imageTypeJPEG)) )? imageTypeJPG : imageTypeJPEG ;
imageURLEnd = ( (imageType == -1) || ((imageTypePNG != -1) && (imageTypePNG < imageType)) )? imageTypePNG : imageType ; //to end the URL with appropriate extention
var imageURL = $.trim(text.substr((imageSearch+8),(imageURLEnd-imageSearch)))
if(imageURL == "" || imageURL.length > 80) //to add default image
{imageURL = "_static/default_thumbnail.png";}
else
{imageURL = '_images' + imageURL;}
{imageURL = '_images' + imageURL;}

//extracting context
textLower = textLower.replace(/<[^<>]*>/gi, ''); //removing the html tags
Expand All @@ -810,12 +810,12 @@ var Search = {
else{
var startPoint = textLower.indexOf('====',0);
var start = startPoint;
var sentenceStart = 0;
var sentenceStart = 0;
$.each(keywords, function() {
var finderStart = startPoint;
var flag = 0;
var firstFound =-1;

while(flag==0 && sentenceStart!=80){
var i = textLower.indexOf(this.toLowerCase(),finderStart);
if (i > -1)
Expand All @@ -825,8 +825,8 @@ var Search = {
sentenceStart=0;
flag =1;
break;}
for( sentenceStart = 0; sentenceStart<80; sentenceStart++){

for( sentenceStart = 0; sentenceStart<80; sentenceStart++){
if((textLower.charAt(start - sentenceStart) == '>') || (textLower.charAt(start - sentenceStart) == '<')) {
finderStart = start+1;
if(finderStart == startPoint)
Expand All @@ -845,14 +845,14 @@ var Search = {

var excerpt = trimmedText.replace(/[`~!@#$%^&*()_|+\-=?:'"<>\{\}\[\]\\\/]/gi, '');
excerpt = '<div class="row"><div class="col-md-3"><img src="' + imageURL + '" style="width: 150px; padding-top: 5px;"></div> <div class="col-md-9" style="padding-top: 20px; padding-left: 0px;">' + excerpt + '</div></div></div>'

var rv = $('<div class="context"></div>').html(excerpt);
$.each(hlwords, function() {
if(this.search('filter')==-1){ //if "filter" is not there in the search query then directly use the query to highlight text
rv = rv.highlightText(this, 'highlighted');
} else { //if "filter" is there in the search query then first strip "filter" from the query and then use it to highlight text
rv = rv.highlightText(this.split('filter')[1], 'highlighted');
}
}
});
return rv;
}
Expand Down