Skip to content

Refactor attachment title#10103

Merged
ahukkanen merged 5 commits intodevelopfrom
fix/refactor-attachment-titles
Mar 30, 2023
Merged

Refactor attachment title#10103
ahukkanen merged 5 commits intodevelopfrom
fix/refactor-attachment-titles

Conversation

@andreslucena
Copy link
Copy Markdown
Member

@andreslucena andreslucena commented Nov 22, 2022

🎩 What? Why?

This is a continuation for #10032. There was one more place where we are displaying the title in an inconsistent manner.

📌 Related Issues

Testing

Check that the related cell is displayed correctly.

♥️ Thank you!

@andreslucena andreslucena added module: core type: fix PRs that implement a fix for a bug labels Nov 22, 2022
ahukkanen
ahukkanen requested changes Dec 20, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

I think this works overall really well but I've found another place for improvement. See this line of code:

decidim/decidim-core/app/cells/decidim/upload_modal/files.erb

Line 13 in a6639f6

<div class="attachment-details" data-title="<%= title_for(attachment) %>" data-filename="<%= file_name_for(attachment) %>" data-state="uploaded">

If you enter a quotation character (") to the attachment title, you can break the HTML. The JS also reads the value from this attribute, so it only reads until the first quotation character.

We can fix this either by escaping the string or using content_tag instead of the plain HTML tag.

Would it be possible to incorporate this fix also to this PR?

@alecslupu alecslupu mentioned this pull request Jan 16, 2023
Merged
@alecslupu alecslupu self-assigned this Feb 15, 2023
@alecslupu
Copy link
Copy Markdown
Contributor

alecslupu commented Mar 17, 2023

Nice work!

I think this works overall really well but I've found another place for improvement. See this line of code:

decidim/decidim-core/app/cells/decidim/upload_modal/files.erb

Line 13 in a6639f6

<div class="attachment-details" data-title="<%= title_for(attachment) %>" data-filename="<%= file_name_for(attachment) %>" data-state="uploaded">

If you enter a quotation character (") to the attachment title, you can break the HTML. The JS also reads the value from this attribute, so it only reads until the first quotation character.

We can fix this either by escaping the string or using content_tag instead of the plain HTML tag.

Would it be possible to incorporate this fix also to this PR?

Hello @ahukkanen , the sanitization has been added in 453d25e.

Now, the content will be rendered as:

        <div class="attachment-details" data-title="An image alert(&#39;ALERT&#39;)" data-filename="Exampledocument.pdf" data-state="uploaded">

@alecslupu alecslupu requested a review from ahukkanen March 17, 2023 23:13
@ahukkanen ahukkanen merged commit e8fac33 into develop Mar 30, 2023
@ahukkanen ahukkanen deleted the fix/refactor-attachment-titles branch March 30, 2023 12:23
@alecslupu alecslupu mentioned this pull request Mar 31, 2023
Merged
entantoencuanto added a commit that referenced this pull request Jun 8, 2023
@entantoencuanto
Merge branch 'feature/redesign' into feature/redesign-pending-assemblies
4ca15a7 
* feature/redesign: (70 commits)
  Fix failing specs
  Redesign sortitions (#10831)
  Redesign: data toggle (#10886)
  Redesign: collaborative drafts (#10729)
  Redesign: pending pages (#10944)
  Redesign: my account (#10904)
  use foundation classes instead of default html validation (#10921)
  Fix failings redesign specs
  Add gitpod support (#10641)
  Fix pipeline after #10409 (#10670)
  Upgrade webpack and other javascript libraries (#10643)
  New Crowdin updates (#10409)
  Refactor attachment title (#10103)
  Fix for exporting hidden moderated proposals (#10630)
  Fix Exception as admin on a Proposal with meeting author (#10628)
  Fix: Deleted and hidden comments are exported (#10629)
  Rename "terms and conditions" to "terms of service" (#10614)
  Upgrade Graphql to 2.0.19 and Graphql-Api to 3.0.1 (#10606)
  Standardize the format of the words "they will" (#10617)
  Fix the spec after word standardization (#10624)
  ...
entantoencuanto added a commit that referenced this pull request Jun 12, 2023
@entantoencuanto
Merge branch 'feature/redesign' into feature/redesign-processes-assem…
0c093ff 
…blies-details-page

* feature/redesign: (71 commits)
  Unskip tests (#10951)
  Fix failing specs
  Redesign sortitions (#10831)
  Redesign: data toggle (#10886)
  Redesign: collaborative drafts (#10729)
  Redesign: pending pages (#10944)
  Redesign: my account (#10904)
  use foundation classes instead of default html validation (#10921)
  Fix failings redesign specs
  Add gitpod support (#10641)
  Fix pipeline after #10409 (#10670)
  Upgrade webpack and other javascript libraries (#10643)
  New Crowdin updates (#10409)
  Refactor attachment title (#10103)
  Fix for exporting hidden moderated proposals (#10630)
  Fix Exception as admin on a Proposal with meeting author (#10628)
  Fix: Deleted and hidden comments are exported (#10629)
  Rename "terms and conditions" to "terms of service" (#10614)
  Upgrade Graphql to 2.0.19 and Graphql-Api to 3.0.1 (#10606)
  Standardize the format of the words "they will" (#10617)
  ...
@alecslupu alecslupu mentioned this pull request Jul 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahukkanen ahukkanen ahukkanen approved these changes

Assignees

@alecslupu alecslupu

Labels

module: core type: fix PRs that implement a fix for a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@andreslucena @alecslupu @ahukkanen