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

Add append info to payload #1109

Merged
merged 3 commits into from
Mar 15, 2017
Merged

Add append info to payload #1109

merged 3 commits into from
Mar 15, 2017

Conversation

ItsGenis
Copy link
Contributor

super
payload[:user_id] = try(:current_user).try(:id)
payload[:organization_id] = try(:current_organization).try(:id)
payload[:app] = try(:current_organization).name
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need try in any of these cases

@@ -9,6 +9,18 @@ class ApplicationController < ActionController::Base
helper Decidim::TranslationsHelper
helper Decidim::DecidimFormHelper
helper Decidim::ReplaceButtonsHelper

def append_info_to_payload(payload)
Copy link
Contributor

Choose a reason for hiding this comment

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

This method needs to be in all base/top controllers. Extract it to a concern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not move it to Decidim::ApplicationController? I think everything inherits from this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrcasals Admin & System inherit from ActionController::Base

@ItsGenis ItsGenis force-pushed the feature/json-logs branch 2 times, most recently from 8c03542 to 408dad3 Compare March 13, 2017 14:20
@codecov-io
Copy link

codecov-io commented Mar 13, 2017

Codecov Report

Merging #1109 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1109      +/-   ##
==========================================
+ Coverage   97.17%   97.18%   +<.01%     
==========================================
  Files         413      414       +1     
  Lines        6912     6930      +18     
==========================================
+ Hits         6717     6735      +18     
  Misses        195      195
Impacted Files Coverage Δ
.../app/controllers/decidim/application_controller.rb 96.29% <100%> (+0.14%)
...e/app/controllers/concerns/decidim/payload_info.rb 100% <100%> (ø)
...ontrollers/decidim/admin/application_controller.rb 94.44% <100%> (+0.32%)
...ntrollers/decidim/system/application_controller.rb 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 141a3b1...db59851. Read the comment docs.

Copy link
Contributor

@oriolgual oriolgual left a comment

Choose a reason for hiding this comment

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

Isn't system missing or does it inherit from core app controller?

def append_info_to_payload(payload)
super
payload[:user_id] = current_user.try(:id)
payload[:organization_id] = current_organization.try(:id)
Copy link
Contributor

Choose a reason for hiding this comment

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

No try

def append_info_to_payload(payload)
super
payload[:user_id] = current_user.try(:id)
payload[:organization_id] = current_organization.try(:id)
Copy link
Contributor

Choose a reason for hiding this comment

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

No try

@ItsGenis
Copy link
Contributor Author

I understood we want it removed from our conversation offline.

@ItsGenis ItsGenis force-pushed the feature/json-logs branch from fbb4940 to 565cf58 Compare March 14, 2017 10:47
@mention-bot
Copy link

@Dor3nz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @oriolgual and @mrcasals to be potential reviewers.

@josepjaume
Copy link
Contributor

Niiice.

@josepjaume josepjaume merged commit abaf5f4 into master Mar 15, 2017
@josepjaume josepjaume deleted the feature/json-logs branch March 15, 2017 07:47
Quentinchampenois pushed a commit to Quentinchampenois/decidim that referenced this pull request Mar 8, 2021
* Opening commit 🚧

* Don't display follows, scopes, and types on cards

* Fix offenses

* User super

* Fix tests

* Fix offenses
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants