MNT: Use new RTD search API #462

pllim · 2020-10-12T17:02:28Z

I started this but I am not really involved on the JS side of things here, so if someone else wanna take over, feel free. Fix #459

Will a simple URL search and replace be sufficient, or we need to change other stuff too?

cc @embray

kelle · 2020-10-12T17:06:04Z

Looping in @jonathansick , this sounds like something he may have thoughts on.

stsewd

The response of the API changed, you can take a look at the new response in https://docs.astropy.org/_/api/v2/search/?q=test&project=astropy&version=stable or check the docs at https://docs.readthedocs.io/en/stable/server-side-search.html#api

stsewd · 2020-10-12T17:20:43Z

tutorials/themes/tutorials-theme/static/searchtools.js

@@ -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){
          docResponse = response;
          //limits the maximum results from the documentation to 15
          docResultsCount = (docResponse.results.hits.total>15)? 15 : docResponse.results.hits.total ;


This should be just results.length

stsewd · 2020-10-12T17:20:49Z

tutorials/themes/tutorials-theme/static/searchtools.js

@@ -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){


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

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?

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

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.

:/

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.

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/

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

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

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.

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.

stsewd · 2020-10-12T17:23:57Z

tutorials/themes/tutorials-theme/static/searchtools.js

-            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' +


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

stsewd · 2020-10-12T17:25:30Z

tutorials/themes/tutorials-theme/static/searchtools.js

-            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' +
                              '?highlight=' + window.location.search.split('=')[1] + '">' +
                              docResponse.results.hits.hits[i].fields.title[0] + '</a>' +


this should be results[i].title

stsewd · 2020-10-12T17:27:25Z

tutorials/themes/tutorials-theme/static/searchtools.js

                              '?highlight=' + window.location.search.split('=')[1] + '">' +
                              docResponse.results.hits.hits[i].fields.title[0] + '</a>' +
-                              '<div class="context">' + 
+                              '<div class="context">' +
                              docResponse.results.hits.hits[i].highlight.content[0] +


This should be iterating over results[i].blocks

pllim · 2020-10-14T15:52:22Z

Thanks, @stsewd ! Just want you to know that I received your much appreciated review, but don't have time to follow-up yet.

embray · 2020-10-14T16:18:53Z

Sorry I didn't get to work on this sooner. I guess it's more urgent than I remembered.

embray · 2020-10-14T16:40:43Z

I'm implementing @stsewd 's review comments.

pllim · 2020-10-14T16:43:47Z

Go for it, @embray . I'll hold off. Thank you for your help!

adrn · 2021-09-27T00:40:11Z

Only ~a year late, but new front-end is launching imminently, so closing this. Thanks @pllim and @stsewd, and sorry for losing track of this...

MNT: Use new RTD search API

966a57e

pllim added infrastructure website-related labels Oct 12, 2020

pllim mentioned this pull request Oct 12, 2020

Search: using deprecated API (https://readthedocs.org/api/v2/docsearch/) #459

Closed

stsewd reviewed Oct 12, 2020

View reviewed changes

pllim assigned embray Oct 14, 2020

adrn closed this Sep 27, 2021

pllim deleted the rtd-depre-api branch September 27, 2021 14:26

MNT: Use new RTD search API #462

MNT: Use new RTD search API #462

Uh oh!

Conversation

pllim commented Oct 12, 2020

Uh oh!

kelle commented Oct 12, 2020

Uh oh!

stsewd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 14, 2020

Uh oh!

embray commented Oct 14, 2020

Uh oh!

embray commented Oct 14, 2020

Uh oh!

pllim commented Oct 14, 2020

Uh oh!

adrn commented Sep 27, 2021

Uh oh!

Uh oh!