Repository navigation
[DT-4062] Show PI institution and profile links - #3930
Merged
otchet-broad merged 20 commits intoSep 23, 2026
Merged
otchet-broad merged 20 commits into
otchet-broad merged 20 commits into
Conversation
… page Adds the PI institution row and ORCID / LinkedIn / website icons to the study overview. The page's primary `study` object comes from the Elasticsearch-backed search index, which does not carry these fields, so they are read from the relational store through a new `Study.getById`. `DataSet.getStudyById` now delegates to it rather than duplicating the request; it keeps typing the payload as the data-submission form's editable `Study` shape, so the two callers of one endpoint stay honest about wanting different views of it. The profile links live inside the PI Name row, and StudyInfoTable drops rows whose value is falsy, so that row's presence cannot hinge on piName alone - the index sometimes has no name for a study whose profile links are populated. Links are rendered only for plain http(s) urls. Third of the DT-4062 stack. **Pairs with consent #3051** (otchet-dt-4061-study-pi-details), which adds the pi_institution_id, pi_orcid, pi_linkedin_url and pi_website_url columns and returns them from GET /api/dataset/study/{studyId}. Until that deploys the request succeeds but the fields come back undefined, so the institution row is omitted and no icons render - the page degrades rather than breaking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…udy-pi-details # Conflicts: # src/components/study_details/StudyDetails.tsx # src/hooks/useStudyDetailsData.ts # test/components/study_details/StudyDetails.spec.tsx
Contributor
otchet-broad
marked this pull request as ready for review
September 15, 2026 01:31
otchet-broad
requested review from
fboulnois and
kevinmarete
and
a lite review from Copilot
and removed request for
a team
September 15, 2026 01:31
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Normalize ORCID values before URL detection to handle whitespace and blank values correctly.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds PI institution details and ORCID, LinkedIn, and website links to study overview pages, with relational metadata fallback.
Changes:
- Adds shared relational study retrieval and metadata fallback.
- Displays PI institution and validated external profile links.
- Expands study overview tests.
File summaries
| File | Summary |
|---|---|
test/components/study_details/StudyDetails.spec.tsx |
Tests PI metadata and profile-link rendering. |
src/types/model.ts |
Adds PI metadata fields. |
src/libs/ajax/Study.ts |
Adds study lookup API support. |
src/libs/ajax/DataSet.ts |
Delegates study retrieval to the shared API. |
src/hooks/useStudyDetailsData.ts |
Fetches relational study details. |
src/components/study_details/StudyDetails.tsx |
Displays institution and profile icons. |
src/components/study_details/piProfileLinks.ts |
Builds and validates profile URLs. Moderate finding: normalize ORCID values before URL detection (3 votes). |
src/components/study_details/PiExternalProfileIcons.tsx |
Renders accessible external links. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
orcidHref decided whether the submitted value was an identifier or a complete URL by testing the untrimmed string. A padded URL therefore failed startsWith and was pasted onto the orcid.org base - a link to a nonsense path - and a whitespace-only value became a link to the ORCID home page once validateHttpUrl trimmed the base it had been appended to. Trimming first settles both: a blank value is omitted, and a complete URL is recognized whatever surrounds it. Adds a spec for the module. Three of its cases fail against the previous behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both the Autofix commit and the local change rewrote orcidHref to normalize before classifying the value. Kept the Autofix logic: it also strips leading slashes, so "/0000-0002-..." does not yield a doubled path under the orcid.org base, which the local version missed. Carried the local comment over, and added the leading-slash case to the spec.
fboulnois
approved these changes
Sep 15, 2026
fboulnois
left a comment
fboulnois
left a comment
Contributor
There was a problem hiding this comment.
Some minor findings identified with Claude and Codex:
1. Profile link validation checks the scheme but never the host (security):
src/components/study_details/piProfileLinks.ts:39: validateHttpUrl only verifies the URL scheme. Any submitter-supplied https:// URL in piOrcid or piLinkedinUrl is rendered on the public study page under the "ORCID profile" / "LinkedIn profile" label and the iD icon.
This is a link-spoofing surface on a page visible to unauthenticated visitors. Suggested fix: constrain the host to orcid.org / linkedin.com and demote anything else to the generic website kind.
2. ORCID scheme check is case-sensitive
src/components/study_details/piProfileLinks.ts:28: normalized.startsWith('http') misclassifies HTTPS://orcid.org/0000-... as a bare identifier, producing https://orcid.org/HTTPS://orcid.org/0000-....
3. Relational fallbacks use ??, so empty index values win
src/components/study_details/StudyDetails.tsx:64: ?? does not fire on '' or [], so a stale or empty index document suppresses exactly the relational piName / dataTypes the fallback exists to supply.
Three findings from review, all on a page an unauthenticated visitor can read: validateHttpUrl checks only the scheme, so any submitter-supplied https URL in piOrcid or piLinkedinUrl rendered under the "ORCID profile" or "LinkedIn profile" label and the iD icon - a link-spoofing surface where the label is the thing being trusted. The host now has to back the claim; anything else is shown as a plain website rather than dropped, so the submitter's link is not silently lost. Subdomains count, since LinkedIn runs country sites, but the boundary dot is required or notlinkedin.com would pass a bare suffix test. The allowlist lives here rather than in validateHttpUrl, which is shared with unrelated callers. The ORCID scheme test was case-sensitive, so HTTPS://orcid.org/0000-... read as a bare identifier and became orcid.org/HTTPS://orcid.org/0000-... The relational fallbacks used ??, which does not fire on '' or [], so an index document present but empty for a field suppressed exactly the relational value the fallback exists to supply. dataTypes spells out the length check, since an empty array is truthy and || alone would not help. Icons key by href now: a demoted link takes the generic website label, so two of them can share one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex review caught that keying the icons by href only moved the collision: a submitter who puts one URL in two fields, or two links both demoted to the generic website label, still produce duplicate React keys. Each field yields at most one link, so the field it came from is the one value that is unique in every case. It is kept separate from `kind`, which says how the link is presented and repeats after a demotion. The index fallbacks go through a `populated` helper rather than `||`. Same semantics, one length check covering '' and [] instead of a bare || plus a special-cased ternary, and it keeps `??` - which `||` on a nullable string would have put in front of Sonar's S6606. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review reported that dataTypes can arrive null and blank the page. The `populated` helper added in 39510ce made that worse rather than better: `??` at least falls through on null, while reading .length off it throws, so a single null field crashed the render instead of needing both. `== null` covers null and undefined together and has to be tested first. The helper now wraps both sides of the fallback as well, so the result is undefined rather than null when neither is populated - StudyTitleBadges' `dataTypes = []` default only fires on undefined, which is the original blank-page path the reviewer described. The test casts through never deliberately: the declared types forbid null, and that is the point - they assert a shape for external data rather than guarantee it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
otchet-broad
merged commit Sep 23, 2026
a6e20b2
into
otchet-dt-4062-study-page-frame
12 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses
https://broadworkbench.atlassian.net/browse/DT-4062
Summary
Adds the PI institution row and ORCID / LinkedIn / website icons to the overview.
The page's primary
studycomes from the Elasticsearch-backed search index, which does not carrythese fields, so they are read from the relational store through a new
Study.getById.DataSet.getStudyByIdnow delegates to it instead of duplicating the request, while still typingthe payload as the data-submission form's editable
Studyshape — the two callers of one endpointstay honest about wanting different views of it.
That relational response is also the fallback for study metadata when a study has no datasets and
so has no search-index document, which is why the two changes sit together.
The profile links live inside the PI Name row, and
StudyInfoTabledrops rows whose value isfalsy, so that row's presence cannot hinge on
piNamealone: the index sometimes has no name fora study whose profile links are populated. Only plain http(s) urls are linked.
GET api/dataset/study/{studyId}already exists on consentdevelop, so until #3051 deploys therequest still returns 200 — only the new fields are absent. The institution row is omitted and no
icons render. This is the one branch in the stack that degrades quietly rather than showing an
error state: the endpoint is not new, only the fields on it are.
Have you read Terra's Contributing Guide lately? If not, do that first.