-
Notifications
You must be signed in to change notification settings - Fork 43
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
497 edit page related external links #2490
base: main
Are you sure you want to change the base?
Changes from 20 commits
6516d52
e4f96f5
2051914
a3e932d
e990e40
d848b72
5e6f89c
91a8762
5654ef4
9eb0701
3074cc7
da8e5f3
5d9baf6
152cc15
eb4c58d
8fc0d20
2013220
28ecf3c
4e7e002
61b158b
ce070ed
0ed49c8
8b4b482
e269831
7e210a3
4baa904
36b21e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,6 +83,24 @@ def linking | |
render action: "show" | ||
end | ||
|
||
def update_related_external_links | ||
artefact = resource.artefact | ||
|
||
if params.key?("artefact") | ||
artefact.assign_attributes(permitted_external_links_params) | ||
|
||
if artefact.save | ||
flash[:success] = "Related links updated." | ||
else | ||
flash[:danger] = artefact.errors.full_messages.join("\n") | ||
end | ||
else | ||
flash[:danger] = "There aren't any external related links yet" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't find a test for this branch? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, yes, that's a good point. I've added a test but it might need more thinking about because that scenario never occurs with the new interface because it always presents the user with fields even if they are left blank (there's no equivalent to this in the legacy version). Maybe that whole branch isn't really necessary any more. This also relates to the problem of validating the two blank fields which currently doesn't happen but returns a success message even though nothing has changed. 🤔 |
||
end | ||
|
||
redirect_to related_external_links_edition_path(@resource.id) | ||
end | ||
|
||
def confirm_unpublish | ||
if redirect_url.blank? || validate_redirect(redirect_url) | ||
render "secondary_nav_tabs/confirm_unpublish" | ||
|
@@ -217,6 +235,10 @@ def permitted_update_params | |
params.require(:edition).permit(%i[title overview in_beta body major_change change_note]) | ||
end | ||
|
||
def permitted_external_links_params | ||
params.require(:artefact).permit(external_links_attributes: %i[title url id _destroy]) | ||
end | ||
|
||
def require_destroyable | ||
return if @resource.can_destroy? | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,8 @@ def current_tab_name | |
"unpublish" | ||
when "admin" | ||
"admin" | ||
when "related_external_links" | ||
"linking" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might just be me but the name 'linking' seems a little different to the other tabs. Would 'external_links' be more in keeping with the other tab names and match up more with the name of the property in the Artefact model? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah that's been bugging me a bit since the start. I took the name from the routes file that appears to redirect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we can use whatever we want here, as it's not really user-visible. So long as we make sure we update all the places that might reference it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've changed |
||
else | ||
"temp_nav_text" | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
<%= render "govuk_publishing_components/components/checkboxes", { | ||
name: "artefact[external_links_attributes][#{index}][_destroy]", | ||
id: "artefact_external_links_attributes_#{index}__destroy", | ||
items: [{label: "Delete", value: "1" }], | ||
} %> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
<% if id %> | ||
<%= hidden_field_tag "artefact[external_links_attributes][#{index}][id]", id %> | ||
<% end %> | ||
|
||
<%= render "govuk_publishing_components/components/input", { | ||
label: { | ||
text: "Title", | ||
bold: true, | ||
}, | ||
name: "artefact[external_links_attributes][#{index}][title]", | ||
id: "artefact_external_links_attributes_#{index}_title", | ||
value: @edition.artefact.external_links[index].present? ? @edition.artefact.external_links[index].title : "", | ||
} %> | ||
|
||
<%= render "govuk_publishing_components/components/input", { | ||
label: { | ||
text: "URL", | ||
bold: true, | ||
}, | ||
name: "artefact[external_links_attributes][#{index}][url]", | ||
id: "artefact_external_links_attributes_#{index}_url", | ||
value: @edition.artefact.external_links[index].present? ? @edition.artefact.external_links[index].url : "", | ||
hint: "Example: https://gov.uk", | ||
} %> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
<div class="govuk-grid-row"> | ||
<div class="govuk-grid-column-two-thirds"> | ||
<%= header_for("Related external links") %> | ||
|
||
<%= form_for @edition.artefact, url: update_related_external_links_edition_path(@resource) do %> | ||
<% | ||
if @edition.artefact.external_links.count == 0 | ||
items = [{ | ||
fields: render(partial: "secondary_nav_tabs/add-another_fieldset", locals: { index: 0, id: nil }), | ||
destroy_checkbox: render(partial: "secondary_nav_tabs/add-another_checkbox", locals: {index: 0}), | ||
}] | ||
empty = render(partial: "secondary_nav_tabs/add-another_fieldset", locals: { index: 1, id: nil }) | ||
else | ||
items = @edition.artefact.external_links.each_with_index.map do | external_link, index | | ||
{ | ||
fields: render(partial: "secondary_nav_tabs/add-another_fieldset", locals: { index:, id: external_link.id }), | ||
destroy_checkbox: render(partial: "secondary_nav_tabs/add-another_checkbox", locals: {index: index}), | ||
} | ||
end | ||
empty = render(partial: "secondary_nav_tabs/add-another_fieldset", locals: { index: @edition.artefact.external_links.count, id: nil }) | ||
end | ||
%> | ||
|
||
<%= render "govuk_publishing_components/components/add_another", { | ||
fieldset_legend: "Link", | ||
add_button_text: "Add another link", | ||
items: items, | ||
empty: empty, | ||
} %> | ||
|
||
<%= render "govuk_publishing_components/components/inset_text", { | ||
text: "After saving, changes to related external links will be visible on the site the next time this publication is published.", | ||
} %> | ||
|
||
<%= render "govuk_publishing_components/components/button", { | ||
text: "Save", | ||
} %> | ||
<% end %> | ||
</div> | ||
</div> |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -625,6 +625,70 @@ class EditionsControllerTest < ActionController::TestCase | |||
end | ||||
end | ||||
|
||||
context "#update_related_external_links" do | ||||
should "display an error message when the title is blank" do | ||||
artefact = @edition.artefact | ||||
|
||||
patch :update_related_external_links, params: { | ||||
id: @edition.id, | ||||
artefact: { | ||||
external_links_attributes: [{ title: "", url: "http://foo-bar.com", _destroy: false }], | ||||
}, | ||||
} | ||||
|
||||
artefact.reload | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I don't think this reload is required, since we're not checking the artefact (and we also wouldn't need the first line either then). |
||||
|
||||
assert_equal "External links is invalid", flash[:danger] | ||||
end | ||||
|
||||
should "display an error message when the url is blank" do | ||||
artefact = @edition.artefact | ||||
|
||||
patch :update_related_external_links, params: { | ||||
id: @edition.id, | ||||
artefact: { | ||||
external_links_attributes: [{ title: "foo", url: "", _destroy: false }], | ||||
}, | ||||
} | ||||
|
||||
artefact.reload | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Same as above. |
||||
|
||||
assert_equal "External links is invalid", flash[:danger] | ||||
end | ||||
|
||||
should "display an error message when the url is invalid" do | ||||
artefact = @edition.artefact | ||||
|
||||
patch :update_related_external_links, params: { | ||||
id: @edition.id, | ||||
artefact: { | ||||
external_links_attributes: [{ title: "foo", url: "an-invalid-url", _destroy: false }], | ||||
}, | ||||
} | ||||
|
||||
artefact.reload | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Same again. |
||||
|
||||
assert_equal "External links is invalid", flash[:danger] | ||||
end | ||||
|
||||
should "update related external links and display a success message when succesfully saved" do | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in "succesfully". |
||||
artefact = @edition.artefact | ||||
|
||||
patch :update_related_external_links, params: { | ||||
id: @edition.id, | ||||
artefact: { | ||||
external_links_attributes: [{ title: "foo", url: "https://foo-bar.com", _destroy: false }], | ||||
}, | ||||
} | ||||
|
||||
artefact.reload | ||||
|
||||
assert_equal "Related links updated.", flash[:success] | ||||
assert_equal "foo", artefact.external_links[0].title | ||||
assert_equal "https://foo-bar.com", artefact.external_links[0].url | ||||
end | ||||
end | ||||
|
||||
private | ||||
|
||||
def description(edition) | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor, but could we move this up a line to retain alphabetical ordering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make sense but I found that some styles for the
gem-c-add-another__remove-button
class get over-written by the genericbutton
styles when they are in that order. That seems to explain the spacing issue you have noted elsewhere WRT the delete button as well. It does seem a little bit unsatisfactory though so I wonder if we should make a change to the component stylesheet to fix this because I should think it's bound to happen again somewhere else.