1059 update designs for multiple endpoints#1173
Conversation
…and summary lists
|
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:
WalkthroughReworked the dataset overview to render active endpoints as ordered summary cards (with URL, documentation, dates and error messages), changed a dataset label, adjusted conditional rendering for authority, updated tests to expect the new "Active endpoints" section, and added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Coverage Report
File CoverageNo changed files found. |
![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)
test/unit/views/organisations/dataset-overview.test.js (1)
40-41:⚠️ Potential issue | 🟡 MinorTest data contains invalid dates.
'2024-19-19'is not a valid date (month 19 doesn't exist). This was likely intended to be something like'2024-09-19'. ThegovukDateTimefilter may produce unexpected output when formatting these invalid dates, potentially masking issues or causing unreliable test behaviour.💚 Suggested fix
{ name: 'endpoint 2', endpoint: 'BAR', endpoint_url: 'http://endpoint2.co.uk', - lastAccessed: '2024-19-19', - lastUpdated: '2024-19-19', + lastAccessed: '2024-09-19', + lastUpdated: '2024-09-19', entryDate: '2024-12-12', error: { code: 404, exception: '' } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/views/organisations/dataset-overview.test.js` around lines 40 - 41, The test fixture in dataset-overview.test.js contains invalid date strings for the fields lastAccessed and lastUpdated ('2024-19-19'); update these values to valid ISO-like dates (e.g. '2024-09-19' or another correct YYYY-MM-DD) so the govukDateTime filter and any date formatting in the test exercise real, valid dates; locate the object/fixture that sets lastAccessed and lastUpdated in the test and replace the invalid month with a valid month value.
🧹 Nitpick comments (2)
src/views/organisations/dataset-overview.html (2)
137-148: Consider simplifying the falsy check fordocumentation_url.The current check
is not defined or endpoint.documentation_url == ""may not catchnullvalues. In Nunjucks, you can use a simpler falsy check:♻️ Suggested simplification
- {% if endpoint.documentation_url is not defined or endpoint.documentation_url == "" %} + {% if not endpoint.documentation_url %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/organisations/dataset-overview.html` around lines 137 - 148, The template currently checks "endpoint.documentation_url is not defined or endpoint.documentation_url == ''" which misses null/other falsy values; replace that conditional with a single falsy check (e.g., "if not endpoint.documentation_url") to cover undefined, null, and empty string, leaving the endpointRows.push usage and the else branch that builds the anchor (using urlStyle and endpoint.documentation_url) unchanged so the error markup and link rendering remain identical.
13-20: Macro always renders error message regardless of error state.The
endpointUrlInfomacro unconditionally renders the error message block (lines 16-18). While current usage at line 127 is guarded by{% if endpoint.error %}, the macro name doesn't indicate it's error-specific, which could lead to misuse if called elsewhere.Consider either:
- Renaming to
endpointUrlWithErrorto clarify intent, or- Adding a guard within the macro
♻️ Option 2: Add guard within macro
{% macro endpointUrlInfo(organisation, dataset, endpoint) %} <a style="{{ urlStyle}}" href="{{endpoint.endpoint_url}}">{{endpoint.endpoint_url}}</a> - - <p class="govuk-error-message"> - {{ endpointErrorMessage(endpoint) }} - </p> - + {% if endpoint.error %} + <p class="govuk-error-message"> + {{ endpointErrorMessage(endpoint) }} + </p> + {% endif %} {% endmacro %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/organisations/dataset-overview.html` around lines 13 - 20, The macro endpointUrlInfo currently always renders the error block; update the macro so the error message paragraph (the call to endpointErrorMessage(endpoint)) is only rendered when endpoint.error is truthy by wrapping that <p> block with a conditional (e.g., {% if endpoint.error %} ... {% endif %}); keep the macro name or optionally rename to endpointUrlWithError if you prefer an explicit name, but ensure endpointUrlInfo only outputs the error markup when endpoint.error is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/views/organisations/dataset-overview.html`:
- Around line 154-157: The template is inconsistent: endpoint.lastAccessed is
not null-guarded before passing to the govukDateTime filter like entryDate and
lastUpdated; update the push into endpointRows so that value.text uses the same
guard pattern (e.g., endpoint.lastAccessed and endpoint.lastAccessed |
govukDateTime) to avoid calling govukDateTime with undefined while leaving
endpointRows, endpoint.lastAccessed and govukDateTime referenced exactly as in
the diff.
---
Outside diff comments:
In `@test/unit/views/organisations/dataset-overview.test.js`:
- Around line 40-41: The test fixture in dataset-overview.test.js contains
invalid date strings for the fields lastAccessed and lastUpdated ('2024-19-19');
update these values to valid ISO-like dates (e.g. '2024-09-19' or another
correct YYYY-MM-DD) so the govukDateTime filter and any date formatting in the
test exercise real, valid dates; locate the object/fixture that sets
lastAccessed and lastUpdated in the test and replace the invalid month with a
valid month value.
---
Nitpick comments:
In `@src/views/organisations/dataset-overview.html`:
- Around line 137-148: The template currently checks "endpoint.documentation_url
is not defined or endpoint.documentation_url == ''" which misses null/other
falsy values; replace that conditional with a single falsy check (e.g., "if not
endpoint.documentation_url") to cover undefined, null, and empty string, leaving
the endpointRows.push usage and the else branch that builds the anchor (using
urlStyle and endpoint.documentation_url) unchanged so the error markup and link
rendering remain identical.
- Around line 13-20: The macro endpointUrlInfo currently always renders the
error block; update the macro so the error message paragraph (the call to
endpointErrorMessage(endpoint)) is only rendered when endpoint.error is truthy
by wrapping that <p> block with a conditional (e.g., {% if endpoint.error %} ...
{% endif %}); keep the macro name or optionally rename to endpointUrlWithError
if you prefer an explicit name, but ensure endpointUrlInfo only outputs the
error markup when endpoint.error is set.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fc18acc4-9ea0-4837-879b-eeb040ad276c
📒 Files selected for processing (4)
.gitignoresrc/views/organisations/dataset-overview.htmltest/acceptance/dataset_overview.test.jstest/unit/views/organisations/dataset-overview.test.js
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
…github.com/digital-land/submit into 1059-update-designs-for-multiple-endpoints
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
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 `@src/views/organisations/dataset-overview.html`:
- Around line 131-146: Replace the unsafe string concatenation with Nunjucks
variable capture and escaping: use a truthy check (e.g., "if not
endpoint.documentation_url" and "if not endpoint.endpoint_url") so null and
empty values are handled, then capture escaped values into locals (e.g., set
doc_url = endpoint.documentation_url | default('') | e and set ep_url =
endpoint.endpoint_url | default('') | e) and use those captured variables for
both href and link text when pushing into endpointRows; also ensure urlStyle is
not injected from user input or is escaped before use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 49e43e75-4d31-47cd-9688-c4290b6525ff
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/views/organisations/dataset-overview.html
…github.com/digital-land/submit into 1059-update-designs-for-multiple-endpoints
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/views/organisations/dataset-overview.html (1)
13-20: Minor: Inconsistent spacing in template variable syntax.The spacing within Nunjucks template variables is inconsistent. Line 14 uses
{{ urlStyle}}and{{endpoint.endpoint_url}}whilst other parts of the template use consistent spacing like{{ urlStyle }}.This is a minor style nit but worth keeping consistent for readability.
♻️ Suggested fix for consistency
{% macro endpointUrlWithError(organisation, dataset, endpoint) %} - <a style="{{ urlStyle}}" href="{{endpoint.endpoint_url}}">{{endpoint.endpoint_url}}</a> + <a style="{{ urlStyle }}" href="{{ endpoint.endpoint_url }}">{{ endpoint.endpoint_url }}</a> <p class="govuk-error-message"> {{ endpointErrorMessage(endpoint) }} </p> {% endmacro %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/organisations/dataset-overview.html` around lines 13 - 20, The template macro endpointUrlWithError has inconsistent Nunjucks variable spacing; update all mustache tags inside that macro to use consistent spacing (e.g., "{{ urlStyle }}", "{{ endpoint.endpoint_url }}", "{{ endpointErrorMessage(endpoint) }}") so every template variable follows the same "{{ var }}" style—edit the occurrences of urlStyle, endpoint.endpoint_url, and endpointErrorMessage within the endpointUrlWithError macro to apply the spaced format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/views/organisations/dataset-overview.html`:
- Around line 13-20: The template macro endpointUrlWithError has inconsistent
Nunjucks variable spacing; update all mustache tags inside that macro to use
consistent spacing (e.g., "{{ urlStyle }}", "{{ endpoint.endpoint_url }}", "{{
endpointErrorMessage(endpoint) }}") so every template variable follows the same
"{{ var }}" style—edit the occurrences of urlStyle, endpoint.endpoint_url, and
endpointErrorMessage within the endpointUrlWithError macro to apply the spaced
format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 77db506b-20f7-4f3f-9e03-dbcedd711fb2
📒 Files selected for processing (1)
src/views/organisations/dataset-overview.html
2 participants
Description
Implements issue #1059 by updating the dataset overview endpoint presentation to the GOV.UK summary card pattern for multiple endpoints, with clearer status/error visibility.
What changed
Acceptance criteria mapping
What type of PR is this? (check all applicable)
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Before
After
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
QA sign off
[optional] Are there any post-deployment tasks we need to perform?
None expected.
[optional] Are there any dependencies on other PRs or Work?
No direct dependencies identified.
If you want, I can also generate a shorter version of this for a cleaner PR body and a separate reviewer checklist comment you can post after opening the PR.
Summary by CodeRabbit
New Features
Tests
Chores