Fix broken dashboard action logs under certain conditions

[ahukkanen]

🎩 What? Why?

There is a programming mistake in most action log presenters that can cause the action log views to break when the version record is not available (for one reason or another) and the diff is set to be shown.

If you take a look at the has_diff? method in the BasePresenter, it tries to assume that the action log version record is always available in order to show its changeset:

decidim/decidim-core/app/presenters/decidim/log/base_presenter.rb

Lines 201 to 203 in 30abdf5

	def has_diff?
	%w(update create).include?(action.to_s) && action_log.version.present?
	end

However, this check is bypassed by most of the action log record specific presenters, such as the proposal presenter as you can see here:

decidim/decidim-proposals/app/presenters/decidim/proposals/admin_log/proposal_presenter.rb

Lines 45 to 47 in 30abdf5

	def has_diff?
	action == "answer" || super
	end

So, if for one reason or another, the action log's version record is not set properly, is deleted, etc. the dashboard index page and the action logs page could be totally broken.

Of course, this does not happen under "normal" and "stable" conditions when everything has been stored normally but it is certainly possible that the version record is not always available for the action log.

This ensures this is always the case and adds a shared example spec which is applied to all of these records that may have had this problem before. If you run those tests against the current core, they would fail.

The exception that you may see in the application logs should look something like this:

Show stacktrace

F, [2020-11-12T10:06:21.384159 #13416] FATAL -- : [a1bcd234-5e67-890a-bc12-34d56efa789b] ActionView::Template::Error (undefined method `changeset' for nil:NilClass):
F, [2020-11-12T10:06:21.384256 #13416] FATAL -- : [a1bcd234-5e67-890a-bc12-34d56efa789b]      8:     <% if logs.any? %>
[a1bcd234-5e67-890a-bc12-34d56efa789b]      9:       <ul class="logs table">
[a1bcd234-5e67-890a-bc12-34d56efa789b]     10:         <% logs.each do |log| %>
[a1bcd234-5e67-890a-bc12-34d56efa789b]     11:           <%= render_log(log) %>
[a1bcd234-5e67-890a-bc12-34d56efa789b]     12:         <% end %>
[a1bcd234-5e67-890a-bc12-34d56efa789b]     13:       </ul>
[a1bcd234-5e67-890a-bc12-34d56efa789b]     14:     <% else %>
F, [2020-11-12T10:06:21.384285 #13416] FATAL -- : [a1bcd234-5e67-890a-bc12-34d56efa789b]   
F, [2020-11-12T10:06:21.384355 #13416] FATAL -- : [a1bcd234-5e67-890a-bc12-34d56efa789b] /home/decidim/staging/shared/bundle/ruby/2.6.0/bundler/gems/decidim-21e4a981fe35/decidim-core/app/presenters/dec
idim/log/base_presenter.rb:247:in `changeset'
[a1bcd234-5e67-890a-bc12-34d56efa789b] /home/decidim/staging/shared/bundle/ruby/2.6.0/bundler/gems/decidim-21e4a981fe35/decidim-core/app/presenters/decidim/log/base_presenter.rb:131:in `diff_presenter'
[a1bcd234-5e67-890a-bc12-34d56efa789b] /home/decidim/staging/shared/bundle/ruby/2.6.0/bundler/gems/decidim-21e4a981fe35/decidim-core/app/presenters/decidim/log/base_presenter.rb:145:in `present_diff'
[a1bcd234-5e67-890a-bc12-34d56efa789b] /home/decidim/staging/shared/bundle/ruby/2.6.0/bundler/gems/decidim-21e4a981fe35/decidim-core/app/presenters/decidim/log/base_presenter.rb:155:in `block in presen
t_action_log'
[a1bcd234-5e67-890a-bc12-34d56efa789b] actionview (5.2.4.4) lib/action_view/helpers/capture_helper.rb:41:in `block in capture'
[a1bcd234-5e67-890a-bc12-34d56efa789b] actionview (5.2.4.4) lib/action_view/helpers/capture_helper.rb:205:in `with_output_buffer'
[a1bcd234-5e67-890a-bc12-34d56efa789b] actionview (5.2.4.4) lib/action_view/helpers/capture_helper.rb:41:in `capture'
[a1bcd234-5e67-890a-bc12-34d56efa789b] actionview (5.2.4.4) lib/action_view/helpers/tag_helper.rb:272:in `content_tag'
[a1bcd234-5e67-890a-bc12-34d56efa789b] /home/decidim/staging/shared/bundle/ruby/2.6.0/bundler/gems/decidim-21e4a981fe35/decidim-core/app/presenters/decidim/log/base_presenter.rb:153:in `present_action_
log'
[a1bcd234-5e67-890a-bc12-34d56efa789b] /home/decidim/staging/shared/bundle/ruby/2.6.0/bundler/gems/decidim-21e4a981fe35/decidim-core/app/presenters/decidim/log/base_presenter.rb:43:in `present'

Testing

• Create a Decidim instance with the default seed content
• Go to one of the proposals components and answer to a proposal
• Run the following in the Rails console: Decidim::ActionLog.last.update!(version_id: nil)
• Go to the dashboard index view as an admin

📋 Checklist

• ❓ 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.

[mrcasals]

Maybe it's better to simply specify the whole class & module names?

describe Decidim::Assemblies::AdminLog::AssembliesTypePresenter do

Same for the rest of the files

[ahukkanen]

@mrcasals At some point I got Rubocop violation for doing that but I'm not sure if the rules have changed.

[ahukkanen]

@mrcasals I tried to change it like this and this is what I get:

[mrcasals]

That's weird, because we have specs similar to what you post:

decidim/decidim-assemblies/spec/presenters/decidim/assemblies/admin_log/value_types/member_position_presenter_spec.rb

Line 5 in d113f06

describe Decidim::Assemblies::AdminLog::ValueTypes::MemberPositionPresenter, type: :helper do

Maybe your Rubocop is not getting the Decidim config for some reason?

[ahukkanen]

No, it's using the correct config. Other cops work totally fine.

It seems to be an issue with the VSCode extension:
rubyide/vscode-ruby#625

Althrough I don't know where it's coming from because what that extension is doing is running the same rubocop that I can run on the command line...

[ahukkanen]

@mrcasals It's now changed as you proposed.

I assume the issue is in rubocop-rspec, I reported it here with further details:
rubocop/rubocop-rspec#1091

The result of that cop depends where rubocop is run from which doesn't seem right to me. The CI runs it in the root of the project when it works correctly. The VSCode extension runs it in the same folder where the file is located at when it does not work.

[mrcasals]

Ah, nice catch, thanks!!

[mrcasals]

Apart from that comment, thank you very much for those PRs, @ahukkanen! Clear, to the point and easy to understand what's going on! As an ex-member of the Core team, I find this kind of PRs super helpful and easy for reviewers 😃 Thanks!

[tramuntanal]

Great job @ahukkanen!