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

Change default OLS viewer to OLS4 #922

Merged
merged 15 commits into from Jul 11, 2023
Merged

Change default OLS viewer to OLS4 #922

merged 15 commits into from Jul 11, 2023

Conversation

matentzn
Copy link
Contributor

@matentzn matentzn commented Jun 2, 2023

@matentzn matentzn marked this pull request as draft June 2, 2023 11:55
@jamesaoverton
Copy link
Member

The double percent signs are important, because this string is being run through Python string formatting, which also uses percent signs (%s). I guess we could switch to Python f-strings for better readability.

This line in the tests will also need to be updated:

ols = 'https://www.ebi.ac.uk/ols/ontologies/%s/terms?iri=http%%3A%%2F%%2Fpurl.obolibrary.org%%2Fobo%%2F'

tools/test.py Outdated Show resolved Hide resolved
IDSPACE should be CHEMINF. The tests should have caught this. ☹️
@matentzn
Copy link
Contributor Author

@lschriml @allysonlister and Robert Thacker have confirmed, @dosumis not (but I know he wants this).

@mcourtot I am also sure will agree, so I will take the blame for that. Remains the blocking OLS issue above!

@mcourtot
Copy link
Contributor

Didn't realize this was pending confirmation - all good to me, thank you!

tools/translate_yaml.py Outdated Show resolved Hide resolved
@matentzn matentzn marked this pull request as ready for review June 30, 2023 14:50
tools/test.py Outdated Show resolved Hide resolved
@matentzn
Copy link
Contributor Author

Ok last problem, no idea whats that about:
https://github.com/OBOFoundry/purl.obolibrary.org/actions/runs/5424480866/jobs/9863896909?pr=922

Seems that the OLS PURL send back a HTTP 303. @serjoshua @udp - any idea why that is? If you click on the URLs they resolve, albeit a bit indirectly and slowly..

@jamesaoverton
Copy link
Member

Don't worry about the fact that the code is "303". There was a decision many years ago to use "303 See Other" for term IRIs.

The problem is that the expected and actual URLs aren't matching up. Not sure why.

tools/test.py Show resolved Hide resolved
tools/test.py Outdated Show resolved Hide resolved
@@ -176,8 +176,7 @@ def process_product(i, product):


ontobee = 'http://www.ontobee.org/browser/rdf.php?o=%s&iri=http://purl.obolibrary.org/obo/'
ols = 'https://www.ebi.ac.uk/ols/ontologies/%s/terms?iri=http%%3A%%2F%%2Fpurl.obolibrary.org%%2Fobo%%2F'

ols = 'https://www.ebi.ac.uk/ols4/ontologies/%s/entities/http%%253A%%252F%%252Fpurl.obolibrary.org%%252Fobo%%252F'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jamesaoverton I got the tests to pass. Note that I am a bit confused why I needed to double encode the entity IRI here to get it to work, but I think its because they moved from "URL parameter" syntax (?iri=) to "URL path" syntax.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, moving from the query string into the path seems to require the extra escape. It's very ugly but it seems to work fine.

Copy link

@serjoshua serjoshua Aug 1, 2023

Choose a reason for hiding this comment

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

I was just reviewing some OLS4 tickets and stumbled upon this. We do support ?iri=, ?shortForm= and ?oboId= (?curie=) for /terms and /entities. What we do not support on the frontend is ?id=. If you encounter any discrepancies, let us know. Thanks.

Copy link
Member

@jamesaoverton jamesaoverton left a comment

Choose a reason for hiding this comment

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

I tested locally and convinced myself that the extra escapes are required.

I'll merge this tomorrow morning when we're both at our desks.

@jamesaoverton jamesaoverton merged commit ae36df4 into master Jul 11, 2023
1 check passed
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.

Switch all term_browser: ols PURLs to OLS4
4 participants