From c919caaa1240e072c19db811791735973aa58862 Mon Sep 17 00:00:00 2001 From: Robbie Court Date: Thu, 3 Sep 2026 22:48:22 +0000 Subject: [PATCH] Link FindStocks and FindComboPublications identifiers out The stock and publication reports rendered every identifier as dead text, so getting from a result to the stock centre or the paper meant copying a number into a search box. FindStocks now emits three links, matching what FlyBase's own stock report offers: the FBst id resolves to its FlyBase report, the stock number resolves to the centre's own catalogue entry, and the collection name resolves to the centre's homepage. FindComboPublications gets the same treatment for the FBrf, DOI, PMID and PMCID. Genotype, title and citation stay plain text -- FlyBase notation is full of brackets and must not be parsed as markdown. The centre URLs come from chado's stockcollectionprop (homepage_url, order_url), read once per process, so the list cannot drift from FlyBase. Only Bloomington's per-stock pattern is hard-coded: its order_url is the batch-order cart, not a per-stock page. FlyORF's order_url is a per-line query prefix, so the stock number appends to it. The other five centres offer only a search box and FlyBase leaves their stock numbers unlinked too, so we do the same rather than invent URL patterns that would rot. Parentheses are percent-encoded in link targets: MarkdownLinkComponent constrains the target to [^()[\]] so that labels may contain brackets, and a DOI like 10.1002/(SICI)... would otherwise terminate the match early. Verified against chado for one stock per collection and for FBgn0020238. --- src/test/test_flybase_combo_pubs.py | 17 +++- src/test/test_flybase_stocks.py | 70 +++++++++++++++- src/vfbquery/flybase_stocks.py | 51 ++++++++++++ src/vfbquery/vfb_queries.py | 121 ++++++++++++++++++++++++---- 4 files changed, 236 insertions(+), 23 deletions(-) diff --git a/src/test/test_flybase_combo_pubs.py b/src/test/test_flybase_combo_pubs.py index 55ab688..9f1d2a2 100644 --- a/src/test/test_flybase_combo_pubs.py +++ b/src/test/test_flybase_combo_pubs.py @@ -113,8 +113,21 @@ def test_fbrf_is_a_visible_column(self): # fbrf must be a normal displayed column, not the (hidden) identity. result = get_flybase_combo_pubs(KNOWN_COMBO_ID, return_dataframe=False, limit=3) fbrf = result["headers"]["fbrf"] - assert fbrf["type"] == "text" + assert fbrf["type"] == "markdown" assert fbrf["order"] >= 0 # FBco0000052 has publications, so an empty result is a defect. assert result["rows"], "KNOWN_COMBO_ID should have publications" - assert result["rows"][0]["id"] == result["rows"][0]["fbrf"] + # the hidden identity stays the bare FBrf; the visible column links it + row = result["rows"][0] + assert row["fbrf"] == f"[{row['id']}](https://flybase.org/reports/{row['id']})" + + @pytest.mark.integration + def test_identifier_columns_link_out(self): + result = get_flybase_combo_pubs(KNOWN_COMBO_ID, return_dataframe=False, limit=5) + for col in ("fbrf", "doi", "pmid", "pmcid"): + assert result["headers"][col]["type"] == "markdown", col + # title and citation are free text and must not be linkified + assert result["headers"]["title"]["type"] == "text" + assert result["headers"]["miniref"]["type"] == "text" + dois = [r["doi"] for r in result["rows"] if r["doi"]] + assert any(d.startswith("[") and "](https://doi.org/" in d for d in dois), dois diff --git a/src/test/test_flybase_stocks.py b/src/test/test_flybase_stocks.py index ae7e6d0..47979d4 100644 --- a/src/test/test_flybase_stocks.py +++ b/src/test/test_flybase_stocks.py @@ -2,7 +2,12 @@ import pytest from vfbquery.flybase_stocks import resolve_entity, find_stocks -from vfbquery.vfb_queries import get_flybase_stocks +from vfbquery.vfb_queries import ( + _flybase_report_url, + _md_link, + _stock_number_url, + get_flybase_stocks, +) def assert_single_selection_id(result): @@ -240,10 +245,67 @@ def test_stock_id_is_a_visible_column(self): # stock_id must be a normal displayed column, not the (hidden) identity. result = get_flybase_stocks(self.CONSTRUCT_WITH_STOCKS, return_dataframe=False, limit=3) stock_id = result["headers"]["stock_id"] - assert stock_id["type"] == "text" + assert stock_id["type"] == "markdown" assert stock_id["order"] >= 0 - # and the hidden identity carries the same FBst value - assert result["rows"][0]["id"] == result["rows"][0]["stock_id"] + # the hidden identity stays the bare FBst; the visible column links it + row = result["rows"][0] + assert row["stock_id"] == f"[{row['id']}](https://flybase.org/reports/{row['id']})" + + @pytest.mark.integration + def test_linked_columns_are_declared_markdown(self): + result = get_flybase_stocks(self.CONSTRUCT_WITH_STOCKS, return_dataframe=False, limit=3) + for col in ("stock_id", "stock_number", "collection"): + assert result["headers"][col]["type"] == "markdown", col + # genotype carries FlyBase bracket notation (w[1118]) and must stay text + assert result["headers"]["genotype"]["type"] == "text" + + @pytest.mark.integration + def test_collection_links_to_the_centre_homepage(self): + result = get_flybase_stocks(self.CONSTRUCT_WITH_STOCKS, return_dataframe=False, limit=3) + assert any(row["collection"].startswith("[") and "](" in row["collection"] + for row in result["rows"]) + + +class TestStockLinkouts: + """Unit tests for the linkout helpers — no database access.""" + + def test_flybase_report_url_accepts_any_fb_id(self): + assert _flybase_report_url("FBst0006565") == "https://flybase.org/reports/FBst0006565" + assert _flybase_report_url("FBrf0239740") == "https://flybase.org/reports/FBrf0239740" + + def test_flybase_report_url_rejects_non_ids(self): + assert _flybase_report_url("6565") is None + assert _flybase_report_url("") is None + assert _flybase_report_url(None) is None + + def test_md_link_without_url_is_plain_text(self): + assert _md_link("6565", None) == "6565" + assert _md_link("", "https://example.org/") == "" + + def test_md_link_escapes_parentheses_in_the_url(self): + # MarkdownLinkComponent's link target excludes ()[] so the label may + # contain brackets; a DOI with parentheses must not end the match early. + assert _md_link("10.1002/(SICI)1096", "https://doi.org/10.1002/(SICI)1096") == \ + "[10.1002/(SICI)1096](https://doi.org/10.1002/%28SICI%291096)" + + def test_bloomington_stock_number_deep_links(self): + assert _stock_number_url("Bloomington Drosophila Stock Center", "6565") == \ + "https://bdsc.indiana.edu/stocks/6565" + + def test_unknown_collection_has_no_stock_number_link(self, monkeypatch): + import vfbquery.flybase_stocks as fbs + monkeypatch.setattr(fbs, "collection_links", + lambda: {"Kyoto Stock Center": { + "order_url": "https://kyotofly.kit.jp/cgi-bin/stocks/index.cgi"}}) + assert _stock_number_url("Kyoto Stock Center", "103972") is None + + def test_order_url_ending_in_equals_takes_the_stock_number(self, monkeypatch): + import vfbquery.flybase_stocks as fbs + monkeypatch.setattr(fbs, "collection_links", + lambda: {"FlyORF": { + "order_url": "https://www.flyorf.ch/imlskonakart/SelectProd.do?flylineId="}}) + assert _stock_number_url("FlyORF", "F000748") == \ + "https://www.flyorf.ch/imlskonakart/SelectProd.do?flylineId=F000748" class TestFindStocksEdgeCases: diff --git a/src/vfbquery/flybase_stocks.py b/src/vfbquery/flybase_stocks.py index b266f97..4e50cad 100644 --- a/src/vfbquery/flybase_stocks.py +++ b/src/vfbquery/flybase_stocks.py @@ -519,3 +519,54 @@ def find_stocks(feature_id, collection_filter=None): return df.to_dict(orient="records") finally: conn.close() + + +# --------------------------------------------------------------------------- +# Stock centre linkouts +# --------------------------------------------------------------------------- + +# Chado carries the stock centres' own URLs in stockcollectionprop +# (homepage_url / order_url / request_text), which is what FlyBase's stock +# report renders. Read them rather than hard-coding a list that goes stale. +_COLLECTION_LINKS_SQL = """ +SELECT sc.uniquename AS collection, + c.name AS prop, + scp.value AS value +FROM stockcollection sc +JOIN stockcollectionprop scp ON sc.stockcollection_id = scp.stockcollection_id +JOIN cvterm c ON scp.type_id = c.cvterm_id +WHERE c.name IN ('homepage_url', 'order_url') +""" + +_collection_links_cache = None + + +def collection_links(): + """Return ``{collection_uniquename: {"homepage_url": ..., "order_url": ...}}``. + + Read once per process from chado's ``stockcollectionprop``. There are seven + collections and the values change about never, so a process-lifetime cache + is enough. A chado failure degrades to an empty map — callers then emit + plain text instead of a link, which is the pre-linkout behaviour. + + :return: dict keyed by stockcollection uniquename + """ + global _collection_links_cache + if _collection_links_cache is not None: + return _collection_links_cache + + links = {} + try: + conn = get_connection(statement_timeout_ms=15000) + try: + df = _run_query(conn, _COLLECTION_LINKS_SQL, {}) + finally: + conn.close() + for _, r in df.iterrows(): + links.setdefault(r["collection"], {})[r["prop"]] = r["value"] + except Exception as e: + print(f"Could not read stock collection links from FlyBase: {e}") + links = {} + + _collection_links_cache = links + return links diff --git a/src/vfbquery/vfb_queries.py b/src/vfbquery/vfb_queries.py index 2a2f5f9..347bcb9 100644 --- a/src/vfbquery/vfb_queries.py +++ b/src/vfbquery/vfb_queries.py @@ -9,7 +9,7 @@ from marshmallow import ValidationError import json import numpy as np -from urllib.parse import unquote +from urllib.parse import quote, unquote import hashlib from .solr_result_cache import (with_solr_cache, solr_caching_disabled, PREVIEW_STATUS_PENDING, PREVIEW_STATUS_COMPLETE, @@ -4944,6 +4944,69 @@ def get_upstream_class_connectivity(short_form: str, return_dataframe=True, limi return {'headers': headers, 'rows': rows, 'count': total_count} +# --------------------------------------------------------------------------- +# FlyBase linkout helpers (FindStocks / FindComboPublications) +# --------------------------------------------------------------------------- + +# Stock centres whose own catalogue has a stable per-stock URL that can be +# built from the FlyBase stock number. Chado carries every centre's homepage +# and order URL in stockcollectionprop, but not this one: Bloomington's +# order_url is its batch-order cart, so the per-stock pattern has to live here. +# FlyBase's own stock report deep-links the stock number for exactly two +# centres -- Bloomington and FlyORF -- and renders it as plain text for the +# other five, which only offer a search box (checked against one stock per +# collection, 2026-09-03). Mirror that rather than invent URL patterns. +_STOCK_NUMBER_URL = { + "Bloomington Drosophila Stock Center": "https://bdsc.indiana.edu/stocks/{number}", +} + +_FLYBASE_ID_RE = re.compile(r"^FB[a-z]{2}\d+$") + + +def _md_link(label, url): + """Build a ``[label](url)`` cell, or plain text when there is no url. + + Parentheses are percent-encoded in the url because the frontend's + MarkdownLinkComponent constrains the link target to ``[^()[\]]+`` so that + labels may contain brackets; a DOI such as ``10.1002/(SICI)...`` would + otherwise terminate the match early. Brackets in the label are left to + :func:`encode_markdown_links`. + """ + if label is None or label == "": + return "" + if not url: + return str(label) + safe_url = str(url).replace("(", "%28").replace(")", "%29") + return f"[{label}]({safe_url})" + + +def _flybase_report_url(fb_id): + """FlyBase report URL for an FB* id, or None if it is not one.""" + if isinstance(fb_id, str) and _FLYBASE_ID_RE.match(fb_id): + return f"https://flybase.org/reports/{fb_id}" + return None + + +def _stock_number_url(collection, number): + """URL for the stock centre's own catalogue entry, or None. + + Bloomington comes from the table above. Every other centre is derived from + chado: FlyORF's ``order_url`` is a per-line query prefix ending in ``=``, + so the stock number appends cleanly; the rest are homepages or batch-order + forms where appending a number would produce a dead link. + """ + if not collection or not number: + return None + pattern = _STOCK_NUMBER_URL.get(collection) + if pattern: + return pattern.format(number=quote(str(number), safe="")) + from .flybase_stocks import collection_links + order_url = collection_links().get(collection, {}).get("order_url") or "" + if order_url.endswith("="): + return order_url + quote(str(number), safe="") + return None + + def get_flybase_stocks(short_form: str, return_dataframe=True, limit: int = -1): """Find available fly stocks from FlyBase for a Feature term. @@ -4962,18 +5025,32 @@ def get_flybase_stocks(short_form: str, return_dataframe=True, limit: int = -1): return pd.DataFrame() return {'headers': {}, 'rows': [], 'count': 0} + from .flybase_stocks import collection_links + homepages = collection_links() + + # Three linkouts, matching what FlyBase's own stock report offers: the FBst + # id resolves to the FlyBase report (which carries the ordering details for + # every centre), the stock number resolves to the centre's own catalogue + # entry where that centre has one, and the collection name resolves to the + # centre's homepage. `id` stays the bare FBst — it is the row's selection + # id, not a rendered cell. rows = [] for s in stocks: + stock_id = s.get('stock_id', '') or '' + stock_number = s.get('stock_number', '') or '' + collection = s.get('collection', '') or '' rows.append({ # Hidden identity column (the FBst the row is about). Without a # `selection_id`-typed column the website consumes the first data # column as the row identity and hides it — which dropped Stock ID # from the table. See the `id` header below. - 'id': s.get('stock_id', ''), - 'stock_id': s.get('stock_id', ''), - 'stock_number': s.get('stock_number', ''), + 'id': stock_id, + 'stock_id': _md_link(stock_id, _flybase_report_url(stock_id)), + 'stock_number': _md_link( + stock_number, _stock_number_url(collection, stock_number)), 'genotype': s.get('genotype', ''), - 'collection': s.get('collection', ''), + 'collection': _md_link( + collection, homepages.get(collection, {}).get('homepage_url')), }) total_count = len(rows) @@ -4985,10 +5062,10 @@ def get_flybase_stocks(short_form: str, return_dataframe=True, limit: int = -1): headers = { 'id': {'title': 'ID', 'type': 'selection_id', 'order': -1}, - 'stock_id': {'title': 'Stock ID', 'type': 'text', 'order': 0}, - 'stock_number': {'title': 'Stock Number', 'type': 'text', 'order': 1}, + 'stock_id': {'title': 'Stock ID', 'type': 'markdown', 'order': 0}, + 'stock_number': {'title': 'Stock Number', 'type': 'markdown', 'order': 1}, 'genotype': {'title': 'Genotype', 'type': 'text', 'order': 2}, - 'collection': {'title': 'Collection', 'type': 'text', 'order': 3}, + 'collection': {'title': 'Collection', 'type': 'markdown', 'order': 3}, } return {'headers': headers, 'rows': rows, 'count': total_count} @@ -5011,21 +5088,31 @@ def get_flybase_combo_pubs(short_form: str, return_dataframe=True, limit: int = return pd.DataFrame() return {'headers': {}, 'rows': [], 'count': 0} + # Same three-linkout treatment as FindStocks: the FBrf resolves to its + # FlyBase report, and DOI / PMID / PMCID resolve to the publisher, PubMed + # and PMC respectively. Title, citation and type stay plain text. rows = [] for p in pubs: + fbrf = p.get('fbrf', '') or '' + doi = p.get('doi', '') or '' + pmid = p.get('pmid', '') or '' + pmcid = p.get('pmcid', '') or '' rows.append({ # Hidden identity column (the FBrf the row is about). Mirrors the # stocks fix: without a `selection_id` column the website consumes # the first data column (FBrf) as the row identity and hides it. - 'id': p.get('fbrf', ''), - 'fbrf': p.get('fbrf', ''), + 'id': fbrf, + 'fbrf': _md_link(fbrf, _flybase_report_url(fbrf)), 'title': p.get('title', ''), 'year': p.get('year', ''), 'miniref': p.get('miniref', ''), 'pub_type': p.get('pub_type', ''), - 'doi': p.get('doi', ''), - 'pmid': p.get('pmid', ''), - 'pmcid': p.get('pmcid', ''), + 'doi': _md_link(doi, f"https://doi.org/{doi}" if doi else None), + 'pmid': _md_link( + pmid, f"https://pubmed.ncbi.nlm.nih.gov/{pmid}/" if pmid else None), + 'pmcid': _md_link( + pmcid, + f"https://www.ncbi.nlm.nih.gov/pmc/articles/{pmcid}/" if pmcid else None), }) total_count = len(rows) @@ -5037,14 +5124,14 @@ def get_flybase_combo_pubs(short_form: str, return_dataframe=True, limit: int = headers = { 'id': {'title': 'ID', 'type': 'selection_id', 'order': -1}, - 'fbrf': {'title': 'FBrf', 'type': 'text', 'order': 0}, + 'fbrf': {'title': 'FBrf', 'type': 'markdown', 'order': 0}, 'title': {'title': 'Title', 'type': 'text', 'order': 1}, 'year': {'title': 'Year', 'type': 'text', 'order': 2}, 'miniref': {'title': 'Reference', 'type': 'text', 'order': 3}, 'pub_type': {'title': 'Type', 'type': 'text', 'order': 4}, - 'doi': {'title': 'DOI', 'type': 'text', 'order': 5}, - 'pmid': {'title': 'PMID', 'type': 'text', 'order': 6}, - 'pmcid': {'title': 'PMCID', 'type': 'text', 'order': 7}, + 'doi': {'title': 'DOI', 'type': 'markdown', 'order': 5}, + 'pmid': {'title': 'PMID', 'type': 'markdown', 'order': 6}, + 'pmcid': {'title': 'PMCID', 'type': 'markdown', 'order': 7}, } return {'headers': headers, 'rows': rows, 'count': total_count}