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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accountability: track results #2206

Merged
merged 21 commits into from Nov 30, 2017
Merged

Accountability: track results #2206

merged 21 commits into from Nov 30, 2017

Conversation

@mrcasals
Copy link
Contributor

@mrcasals mrcasals commented Nov 15, 2017

馃帺 What? Why?

This PR adds traceabilty to results, so end users can track the different versions and see how a result is being updated.

This PR keeps track of changes in all attributes of the result, but does not keep track of the related proposals. paper_trail, the gem I'm using to generate versions, says it can track relations, but I doubt it can track how we're dealing with our linked_resources, and the README says it's an experimental feature and it's not ready for production. This means we have to track it manually, creating a new version manually every time the relations are changed. But we'd also need to use a custom serializer, because otherwise it cannot keep track of the relations since their are not a real attribute.

This leads to a rabbit hole that will end up biting us in the future. Instead of that, I suggest iterating over the current solution and, if needed, create a custom way to save versions that has a way to keep track of our own linked_resources.

In order to solve the issue by now, we could either remove the ability to change the linked proposals of a result when editing, or don't track them at all. Since I understand sometimes human errors can be made and can cause people to forget the linked proposals and edit the result to add them, I think it's good if we don't track the linked proposals at all.

馃搶 Related Issues

馃搵 Subtasks

  • Add paper_trail
  • Show version number in result page
  • Pass the whodunnit everywhere
  • Add views to compare versions
  • Add a view to show all versions of a single result
  • Add diff highlighting as in GitHub
  • Improve navigation
  • Add feature specs

馃摲 Screenshots (optional)

Screenshots are updated with the latest commit.

Result page with versions:

Versions index page for a single result:

Version created after modifying the description of the result:

Version created after creating a result:

@mrcasals mrcasals self-assigned this Nov 15, 2017
@mrcasals mrcasals force-pushed the accountability/track-results branch 7 times, most recently from ab1f8a2 to 5a6dec7 Nov 17, 2017
@codecov
Copy link

@codecov codecov bot commented Nov 17, 2017

Codecov Report

Merging #2206 into master will increase coverage by 0.01%.
The diff coverage is 98.68%.

@@            Coverage Diff            @@
##           master   #2206      +/-   ##
=========================================
+ Coverage   98.58%   98.6%   +0.01%     
=========================================
  Files        1239    1249      +10     
  Lines       28439   28725     +286     
=========================================
+ Hits        28038   28325     +287     
+ Misses        401     400       -1

@mrcasals mrcasals force-pushed the accountability/track-results branch 5 times, most recently from ceb8996 to e00f53c Nov 20, 2017
helper Decidim::TraceabilityHelper
helper_method :current_version, :result

def show
Copy link
Member

@oriolgual oriolgual Nov 20, 2017

You don't need to declare actions

<span class="definition-data__title"><%= t("results.show.stats.current_version_number", scope: "decidim.accountability") %></span>
<%= result.versions.count %>
</div>
<% if result.versions.last.whodunnit.present? %>
Copy link
Member

@oriolgual oriolgual Nov 20, 2017

Maybe add a method to result instead of all this chaining?

<span class="definition-data__title"><%= t("results.show.stats.current_version_number", scope: "decidim.accountability") %></span>
<%= result.versions.count %>
</div>
<% if result.versions.last.whodunnit.present? %>
Copy link
Member

@oriolgual oriolgual Nov 20, 2017

Ditto

<div>
<%= link_to result_versions_path(result, index) do %>
<h6 class="card--list__heading heading6">
VERSION <%= index + 1 %>
Copy link
Member

@oriolgual oriolgual Nov 20, 2017

i18n

<div class="columns section mediumlarge-4 large-3">
<div class="card extra definition-data">
<div class="definition-data__item versions_count">
<span class="definition-data__title">VERSION NUMBER</span>
Copy link
Member

@oriolgual oriolgual Nov 20, 2017

i18n

</div>
<% if current_version.whodunnit.present? %>
<div class="definition-data__item last_revision_by">
<span class="definition-data__title">VERSION AUTHOR</span>
Copy link
Member

@oriolgual oriolgual Nov 20, 2017

i18n

save if changed?
return unless changed?

# rubocop:disable Rails/SkipsModelValidations
Copy link
Member

@oriolgual oriolgual Nov 20, 2017

馃様

Copy link
Contributor Author

@mrcasals mrcasals Nov 20, 2017

Is the documentation clear enough as to why we need to do this? Maybe I should improve it :(

Copy link
Contributor Author

@mrcasals mrcasals Nov 21, 2017

I've improved the documentation to note why we need this.

Copy link
Contributor

@josepjaume josepjaume Nov 22, 2017

It's an ugly hack, but I can't see any ways to work around it. It's PostgreSQL/Rails/MVC's fault I believe...

@mrcasals mrcasals force-pushed the accountability/track-results branch 14 times, most recently from e08ecea to 9dc3c4a Nov 22, 2017
@mrcasals mrcasals force-pushed the accountability/track-results branch from f851088 to 74b6170 Nov 29, 2017
@josepjaume
Copy link
Contributor

@josepjaume josepjaume commented Nov 29, 2017

Codeclimate says no

@mrcasals mrcasals force-pushed the accountability/track-results branch from 74b6170 to 342c50c Nov 29, 2017
@mrcasals
Copy link
Contributor Author

@mrcasals mrcasals commented Nov 29, 2017

@josepjaume solved!

@josepjaume
Copy link
Contributor

@josepjaume josepjaume commented Nov 30, 2017

Let's get this in!

@josepjaume josepjaume merged commit fca4a7a into master Nov 30, 2017
19 checks passed
@ghost ghost removed the in-review label Nov 30, 2017
@josepjaume josepjaume deleted the accountability/track-results branch Nov 30, 2017
@oriolgual oriolgual added this to the CDP1 milestone Dec 10, 2018
@oriolgual oriolgual removed this from the CDP1 milestone Dec 10, 2018
@oriolgual oriolgual added this to the CDP3 milestone Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants