Skip to content

Fix content type delegation to blank attachments#8230

Merged
leio10 merged 2 commits intodevelopfrom
fix/content_type_delegation_to_blank_attachments
Jul 22, 2021
Merged

Fix content type delegation to blank attachments#8230
leio10 merged 2 commits intodevelopfrom
fix/content_type_delegation_to_blank_attachments

Conversation

@entantoencuanto
Copy link
Copy Markdown
Contributor

@entantoencuanto entantoencuanto commented Jul 22, 2021

🎩 What? Why?

This PR avoids some errors produced when a Decidim::Attachment exists but its associated attached file is not present

📌 Related Issues

Testing

Describe the best way to test or validate your PR.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

♥️ Thank you!

@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Jul 22, 2021

I'm not sure if these are relevant but just to make sure I point it out, if you do a global search with file.content_type, you can also find other places that might be affected.

As said, I'm not sure because I'm not that familiar with the Active Storage implementation (yet), but it might be worth to double check.

@entantoencuanto entantoencuanto force-pushed the fix/content_type_delegation_to_blank_attachments branch from e5e0385 to a8c0127 Compare July 22, 2021 13:38
@entantoencuanto
Copy link
Copy Markdown
Contributor Author

entantoencuanto commented Jul 22, 2021

Yes, you are right, I've take a look to other places in the application here .content_type is called, but except other file where errors may appear is decidim-elections/app/views/decidim/elections/elections/show.html.erb I think in the other cases the presence of the file is ensured (for example in the Decidim::Attachment model the presence of the file is checked previously with attached? and in other commands and forms I think that the method is not called if the file is not present). Anyway I will be pending if new errors appear.

@leio10 leio10 changed the title Fix/content type delegation to blank attachments Fix content type delegation to blank attachments Jul 22, 2021
@leio10 leio10 added type: fix PRs that implement a fix for a bug module: core in-review labels Jul 22, 2021
leio10
leio10 approved these changes Jul 22, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@leio10 leio10 left a comment

Choose a reason for hiding this comment

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

Great!

@leio10 leio10 merged commit f9b8e75 into develop Jul 22, 2021
@leio10 leio10 deleted the fix/content_type_delegation_to_blank_attachments branch July 22, 2021 15:13
entantoencuanto added a commit that referenced this pull request Jul 26, 2021
@entantoencuanto
Merge branch 'develop' into 7714-quilljs
e488132 
* develop: (32 commits)
  Remove obsolete rake webpack task (#8237)
  Active storage migrations service (#7902)
  Fix content type delegation to blank attachments (#8230)
  Evote bug fixing (#8220)
  Fix the proposal data migration for proposals without authors or organization (#8015)
  Bump addressable version because security issues (#8229)
  Online meetings iframe visibility with time (#8097)
  Meetings iframe and iframe URL (#8096)
  Remove flaky test on meetings (#8226)
  Fix broken tests after problematic PRs (#8224)
  Apply permissions system to comments (#8035)
  Set current_component as commentable when commentable is a participatory space (#8189)
  Fix don't require inactive authorization handlers (#8122)
  Improve metrics calculations performance (#8215)
  Fix performance issue in notification settings page (#8155)
  Active storage migration (#7598)
  Update manual installation guide in documentation (#8217)
  Load JS configuration in elections focus mode layout (#8213)
  Fix user activity pagination when there are hidden items (#8202)
  Make it possible to define SCSS settings overrides from modules (#8198)
  ...
roxanaopr pushed a commit to i-need-another-coffee/decidim that referenced this pull request Jul 29, 2021
@entantoencuanto @roxanaopr
Fix content type delegation to blank attachments (decidim#8230)
09eecea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@leio10 leio10 leio10 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

in-review 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.

Active Storage bug: content_type delegated to attachment, but attachment is nil

3 participants

@entantoencuanto @ahukkanen @leio10