Add metadata dashboard buttons to designated pages.#951
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a MetadataDashboardButton Glimmer component (template, JS, styles), inserts it into provider and repository show templates, and adds METADATA_DASHBOARD_URL plus fabricaDeployTarget to environment config (test target sets URL to empty). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/environment.js (1)
236-252:⚠️ Potential issue | 🟠 MajorMissing
METADATA_DASHBOARD_URLoverride for production environment.The production environment block sets explicit URLs for
API_URL,FABRICA_URL,SEARCH_URL, etc., but does not setMETADATA_DASHBOARD_URL. This means production will use the default stage URL (https://metadata.stage.datacite.org) instead of a production metadata dashboard URL.If there's a production metadata dashboard, add the appropriate URL:
🐛 Proposed fix (if production URL exists)
if (fabricaDeployTarget === 'production') { ENV.SITE_TITLE = 'DataCite Fabrica'; ENV.API_URL = 'https://api.datacite.org'; ENV.ORCID_URL = 'https://orcid.org'; ENV.FABRICA_URL = 'https://doi.datacite.org'; ENV.CROSSREF_API_URL = 'https://api.crossref.org'; ENV.EVENTDATA_URL = 'https://api.datacite.org'; ENV.SEARCH_URL = 'https://search.datacite.org'; ENV.CDN_URL = process.env.CDN_URL || 'https://assets.datacite.org' ENV.HOME_URL = 'https://datacite.org'; ENV.COOKIE_DOMAIN = '.datacite.org'; + ENV.METADATA_DASHBOARD_URL = 'https://metadata.datacite.org'; // Adjust URL as needed ENV.HANDLE_SERVER = typeof process.env.HANDLE_SERVER === 'undefined' || process.env.HANDLE_SERVER == '' ? 'https://doi.org' : normalizeURL(process.env.HANDLE_SERVER); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/environment.js` around lines 236 - 252, The production block for fabricaDeployTarget omits setting ENV.METADATA_DASHBOARD_URL so production falls back to the stage URL; add a production override by setting ENV.METADATA_DASHBOARD_URL (e.g., to process.env.METADATA_DASHBOARD_URL || 'https://metadata.datacite.org') inside the if (fabricaDeployTarget === 'production') branch so the app uses the correct production metadata dashboard; reference the fabricaDeployTarget check and the ENV object (ENV.METADATA_DASHBOARD_URL) when making the change.
🧹 Nitpick comments (1)
app/components/metadata-dashboard-button.hbs (1)
1-7: Consider handling the case whenmetadataDashboardUrlis empty.When
fabricaDeployTargetis something other than"test"butMETADATA_DASHBOARD_URLis empty or undefined, the button will render with an empty/invalidhref. While the current logic hides the button for the"test"environment, you might want to add an additional check to ensure the URL is valid before rendering:♻️ Proposed enhancement
-{{`#if` (not-eq this.fabricaDeployTarget "test")}} +{{`#if` (and (not-eq this.fabricaDeployTarget "test") this.metadataDashboardUrl)}} <div class="btn-group btn-group-sm metadata-dashboard-button"> <a href={{this.metadataDashboardUrl}} target="_blank" rel="noopener noreferrer" class="btn btn-warning" id="metadata-dashboard"> <i class="fas fa-external-link-alt"></i> Metadata Dashboard </a> </div> {{/if}}Alternatively, if the URL is always guaranteed to be set when not in test environment, the current implementation is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/metadata-dashboard-button.hbs` around lines 1 - 7, The button currently renders whenever this.fabricaDeployTarget !== "test" even if this.metadataDashboardUrl is empty; update the template to also check that this.metadataDashboardUrl is truthy before rendering the anchor (e.g., combine the existing fabri caDeployTarget check with a presence check for this.metadataDashboardUrl using an and/compound helper) so the metadata-dashboard anchor (id "metadata-dashboard") is only output when a valid URL exists, or alternatively render a disabled/hidden button when this.metadataDashboardUrl is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/environment.js`:
- Line 4: Remove the unused import of the minimatch module: delete the line that
declares the constant M from require('minimatch') (the import symbol "M" created
by the statement const M = require('minimatch')). Ensure no other references to
M/minimatch remain in this file before committing so the file has no unused
imports.
---
Outside diff comments:
In `@config/environment.js`:
- Around line 236-252: The production block for fabricaDeployTarget omits
setting ENV.METADATA_DASHBOARD_URL so production falls back to the stage URL;
add a production override by setting ENV.METADATA_DASHBOARD_URL (e.g., to
process.env.METADATA_DASHBOARD_URL || 'https://metadata.datacite.org') inside
the if (fabricaDeployTarget === 'production') branch so the app uses the correct
production metadata dashboard; reference the fabricaDeployTarget check and the
ENV object (ENV.METADATA_DASHBOARD_URL) when making the change.
---
Nitpick comments:
In `@app/components/metadata-dashboard-button.hbs`:
- Around line 1-7: The button currently renders whenever
this.fabricaDeployTarget !== "test" even if this.metadataDashboardUrl is empty;
update the template to also check that this.metadataDashboardUrl is truthy
before rendering the anchor (e.g., combine the existing fabri caDeployTarget
check with a presence check for this.metadataDashboardUrl using an and/compound
helper) so the metadata-dashboard anchor (id "metadata-dashboard") is only
output when a valid URL exists, or alternatively render a disabled/hidden button
when this.metadataDashboardUrl is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2282fc79-c9ad-4eb2-a08f-1eb87456d7e2
📒 Files selected for processing (6)
app/components/metadata-dashboard-button.hbsapp/components/metadata-dashboard-button.jsapp/styles/local.cssapp/templates/providers/show/index.hbsapp/templates/repositories/show/index.hbsconfig/environment.js
svogt0511
temporarily deployed
to
vercel-bracco-preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/environment.js (1)
209-211: NormalizeMETADATA_DASHBOARD_URLfrom env input.At Line 210, env-provided URLs are used raw while defaults are normalized elsewhere. Applying
normalizeURLhere avoids trailing-slash inconsistencies.♻️ Proposed refactor
- METADATA_DASHBOARD_URL: - process.env.METADATA_DASHBOARD_URL || - 'https://metadata.stage.datacite.org' + METADATA_DASHBOARD_URL: + typeof process.env.METADATA_DASHBOARD_URL === 'undefined' || + process.env.METADATA_DASHBOARD_URL == '' + ? 'https://metadata.stage.datacite.org' + : normalizeURL(process.env.METADATA_DASHBOARD_URL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/environment.js` around lines 209 - 211, The METADATA_DASHBOARD_URL value uses the raw env input causing trailing-slash inconsistencies; wrap the environment value with normalizeURL so the assigned value is normalized (e.g., set METADATA_DASHBOARD_URL to normalizeURL(process.env.METADATA_DASHBOARD_URL || 'https://metadata.stage.datacite.org')), and ensure normalizeURL is imported/available in this module before using it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/environment.js`:
- Line 231: The METADATA_DASHBOARD_URL is only cleared when fabricaDeployTarget
=== 'test', which misses Ember test runs where environment === 'test' but
FABRICA_DEPLOY_TARGET is unset; update the config to also set
ENV.METADATA_DASHBOARD_URL = "" when environment === 'test' (in the same block
that handles other test-specific settings) or change the existing condition to
clear the URL when either fabricaDeployTarget === 'test' OR environment ===
'test' so the dashboard is reliably disabled during Ember tests; refer to
ENV.METADATA_DASHBOARD_URL, fabricaDeployTarget (derived from
process.env.FABRICA_DEPLOY_TARGET), and environment to implement the fix.
---
Nitpick comments:
In `@config/environment.js`:
- Around line 209-211: The METADATA_DASHBOARD_URL value uses the raw env input
causing trailing-slash inconsistencies; wrap the environment value with
normalizeURL so the assigned value is normalized (e.g., set
METADATA_DASHBOARD_URL to normalizeURL(process.env.METADATA_DASHBOARD_URL ||
'https://metadata.stage.datacite.org')), and ensure normalizeURL is
imported/available in this module before using it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2523bab-7c71-4c49-9de2-1c38dc9bdcbf
📒 Files selected for processing (1)
config/environment.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/environment.js`:
- Around line 209-212: METADATA_DASHBOARD_URL currently defaults to the stage
URL and is missing the production override; update the fabricaDeployTarget ===
'production' block to set METADATA_DASHBOARD_URL to the production dashboard
(e.g. 'https://metadata.datacite.org') just like
API_URL/SEARCH_URL/HOME_URL/HANDLE_SERVER are overridden there, ensuring
normalizeURL(process.env.METADATA_DASHBOARD_URL) is used if relying on an env
var and falling back to the hardcoded production URL when not provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c9f3489-0f37-4cd7-ac1a-f956e09d541d
📒 Files selected for processing (2)
app/components/metadata-dashboard-button.hbsconfig/environment.js
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/metadata-dashboard-button.hbs
svogt0511
temporarily deployed
to
vercel-bracco-preview
GitHub Actions
Inactive
|
|
||
| export default class MetadataDashboardButton extends Component { | ||
| get metadataDashboardUrl() { | ||
| return ENV.METADATA_DASHBOARD_URL; |
There was a problem hiding this comment.
The link should go to the specific account’s view on the metadata dashboard. Right now, it always goes to https://metadata.stage.datacite.org. Instead, the relevant view on would be https://metadata.stage.datacite.org/{account-id}, or https://metadata.datacite.org/{account-id} on production. This structure will work for both client and provider account types.
There was a problem hiding this comment.
I will take a look!
There was a problem hiding this comment.
For example - account id could be datacite.test, datacite, dc, etc. So for example,
https://metadata.datacite.org/dc
https://metadata.datacite.org/datacite
https://metadata.datacite.org/datacite.test
I have it doing that. I am assuming this is correct. If not, let me know.
A couple of questions:
- What about admin? I assume there should be no button for admin? (I just checked and the button does not show up for the admin user.)
- If staging uses production data, we will end up with a lot of urls in staging that produce '404', 'page not found' errors.
Let me know what you think.
There was a problem hiding this comment.
yes, that's correct!
- It's fine if we don't have a button for admin.
- Yes, that's true. But since we don't share stage.datacite.org with users (only test.datacite.org) I think this is acceptable.
|
Looking good! One question (in addition to Kelly's points above): Correct me if I'm wrong, @KellyStathis , but were we also intending to add the button to the DOIs tab? As in routes like the following: |
Yes that's right! I overlooked this in my review. It should be in both places. |
svogt0511
temporarily deployed
to
vercel-bracco-preview
GitHub Actions
Inactive
…tion admin and client admin.
svogt0511
temporarily deployed
to
vercel-bracco-preview
GitHub Actions
Inactive
3 participants
Purpose
closes: https://github.com/datacite/product-backlog/issues/575
preview: #951
Approach
Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines:
Summary by CodeRabbit