Skip to content
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

Deprecate Attachment#urlname #1848

Merged
merged 1 commit into from
Jul 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/alchemy/admin/pages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ def destroy

def link
@attachments = Attachment.all.collect { |f|
[f.name, download_attachment_path(id: f.id, name: f.urlname)]
[f.name, download_attachment_path(id: f.id, name: f.slug)]
}
end

Expand Down
4 changes: 2 additions & 2 deletions app/helpers/alchemy/url_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ def show_page_path_params(page, optional_params = {})

# Returns the path for downloading an alchemy attachment
def download_alchemy_attachment_path(attachment)
alchemy.download_attachment_path(attachment, attachment.urlname)
alchemy.download_attachment_path(attachment, attachment.slug)
end

# Returns the url for downloading an alchemy attachment
def download_alchemy_attachment_url(attachment)
alchemy.download_attachment_url(attachment, attachment.urlname)
alchemy.download_attachment_url(attachment, attachment.slug)
end

# Returns the full url containing host, page and anchor for the given element
Expand Down
5 changes: 4 additions & 1 deletion app/models/alchemy/attachment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,13 @@ def to_jq_upload
end

# An url save filename without format suffix
def urlname
def slug
CGI.escape(file_name.gsub(/\.#{extension}$/, "").tr(".", " "))
end

alias_method :urlname, :slug
deprecate urlname: :slug, deprecator: Alchemy::Deprecation

# Checks if the attachment is restricted, because it is attached on restricted pages only
def restricted?
pages.any? && pages.not_restricted.blank?
Expand Down
2 changes: 1 addition & 1 deletion app/models/alchemy/essence_file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def attachment_url

routes.download_attachment_path(
id: attachment.id,
name: attachment.urlname,
name: attachment.slug,
format: attachment.suffix,
)
end
Expand Down
2 changes: 1 addition & 1 deletion app/views/alchemy/essences/_essence_file_view.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
attachment.name,
alchemy.download_attachment_path(
attachment,
name: attachment.urlname,
name: attachment.slug,
format: attachment.suffix
),
{
Expand Down
18 changes: 9 additions & 9 deletions spec/helpers/alchemy/url_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ module Alchemy

context "with addiitonal parameters" do
subject(:show_page_path_params) do
helper.show_page_path_params(page, {query: "test"})
helper.show_page_path_params(page, { query: "test" })
end

it "returns a Hash with urlname, no locale and query parameter" do
Expand All @@ -52,7 +52,7 @@ module Alchemy

context "with additional parameters" do
subject(:show_page_path_params) do
helper.show_page_path_params(page, {query: "test"})
helper.show_page_path_params(page, { query: "test" })
end

it "returns a Hash with urlname, locale and query parameter" do
Expand All @@ -74,7 +74,7 @@ module Alchemy
end

it "should return the correct relative path string with additional parameters" do
expect(helper.show_alchemy_page_path(page, {query: "test"})).to \
expect(helper.show_alchemy_page_path(page, { query: "test" })).to \
eq("/#{page.language_code}/testpage?query=test")
end
end
Expand All @@ -89,7 +89,7 @@ module Alchemy
end

it "should return the correct relative path string with additional parameter" do
expect(helper.show_alchemy_page_path(page, {query: "test"})).to \
expect(helper.show_alchemy_page_path(page, { query: "test" })).to \
eq("/testpage?query=test")
end
end
Expand All @@ -107,7 +107,7 @@ module Alchemy
end

it "should return the correct url string with additional parameters" do
expect(helper.show_alchemy_page_url(page, {query: "test"})).to \
expect(helper.show_alchemy_page_url(page, { query: "test" })).to \
eq("http://#{helper.request.host}/#{page.language_code}/testpage?query=test")
end
end
Expand All @@ -123,24 +123,24 @@ module Alchemy
end

it "should return the correct url string with additional parameter" do
expect(helper.show_alchemy_page_url(page, {query: "test"})).to \
expect(helper.show_alchemy_page_url(page, { query: "test" })).to \
eq("http://#{helper.request.host}/testpage?query=test")
end
end
end
end

context "attachment path helpers" do
let(:attachment) { mock_model(Attachment, urlname: "test-attachment.pdf") }
let(:attachment) { mock_model(Attachment, slug: "test-attachment.pdf") }

it "should return the correct relative path to download an attachment" do
expect(helper.download_alchemy_attachment_path(attachment)).to \
eq("/attachment/#{attachment.id}/download/#{attachment.urlname}")
eq("/attachment/#{attachment.id}/download/#{attachment.slug}")
end

it "should return the correct url to download an attachment" do
expect(helper.download_alchemy_attachment_url(attachment)).to \
eq("http://#{helper.request.host}/attachment/#{attachment.id}/download/#{attachment.urlname}")
eq("http://#{helper.request.host}/attachment/#{attachment.id}/download/#{attachment.slug}")
end
end

Expand Down
8 changes: 4 additions & 4 deletions spec/models/alchemy/attachment_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,22 +54,22 @@ module Alchemy
describe "urlname sanitizing" do
it "escapes unsafe url characters" do
attachment.file_name = "f#%&cking cute kitten pic.png"
expect(attachment.urlname).to eq("f%23%25%26cking+cute+kitten+pic")
expect(attachment.slug).to eq("f%23%25%26cking+cute+kitten+pic")
end

it "removes format suffix from end of file name" do
attachment.file_name = "pic.png.png"
expect(attachment.urlname).to eq("pic+png")
expect(attachment.slug).to eq("pic+png")
end

it "converts dots into escaped spaces" do
attachment.file_name = "cute.kitten.pic.png"
expect(attachment.urlname).to eq("cute+kitten+pic")
expect(attachment.slug).to eq("cute+kitten+pic")
end

it "escapes umlauts in the name" do
attachment.file_name = "süßes katzenbild.png"
expect(attachment.urlname).to eq("s%C3%BC%C3%9Fes+katzenbild")
expect(attachment.slug).to eq("s%C3%BC%C3%9Fes+katzenbild")
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/models/alchemy/essence_file_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ module Alchemy
subject { essence.attachment_url }

it "returns the download attachment url." do
is_expected.to match(/\/attachment\/#{attachment.id}\/download\/#{attachment.urlname}\.#{attachment.suffix}/)
is_expected.to match(/\/attachment\/#{attachment.id}\/download\/#{attachment.slug}\.#{attachment.suffix}/)
end

context "without attachment assigned" do
Expand Down
12 changes: 6 additions & 6 deletions spec/views/essences/essence_file_view_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
build_stubbed(:alchemy_attachment, file: file, name: "an image", file_name: "image with spaces.png")
end

let(:essence) { Alchemy::EssenceFile.new(attachment: attachment) }
let(:content) { Alchemy::Content.new(essence: essence) }
let(:essence) { Alchemy::EssenceFile.new(attachment: attachment) }
let(:content) { Alchemy::Content.new(essence: essence) }

context "without attachment" do
let(:essence) { Alchemy::EssenceFile.new(attachment: nil) }
Expand All @@ -27,7 +27,7 @@
it "renders a link to download the attachment" do
render content, content: content
expect(rendered).to have_selector(
"a[href='/attachment/#{attachment.id}/download/#{attachment.urlname}.#{attachment.suffix}']",
"a[href='/attachment/#{attachment.id}/download/#{attachment.slug}.#{attachment.suffix}']"
)
end

Expand All @@ -40,14 +40,14 @@

context "with link_text set in the local options" do
it "has this value as link text" do
render content, content: content, options: {link_text: "Download this file"}
render content, content: content, options: { link_text: "Download this file" }
expect(rendered).to have_selector("a:contains('Download this file')")
end
end

context "with link_text set in the content settings" do
before do
allow(content).to receive(:settings) { {link_text: "Download this file"} }
allow(content).to receive(:settings) { { link_text: "Download this file" } }
end

it "has this value as link text" do
Expand All @@ -69,7 +69,7 @@

context "with html_options given" do
it "renders the linked ingredient with these options" do
render content, content: content, html_options: {title: "Bar", class: "blue"}
render content, content: content, html_options: { title: "Bar", class: "blue" }
expect(rendered).to have_selector('a.blue[title="Bar"]')
end
end
Expand Down