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

Feedback needed after Endorsing when user has no user_groups #2998

Merged

Conversation

tramuntanal
Copy link
Contributor

@tramuntanal tramuntanal commented Mar 14, 2018

…new design.

🎩 What? Why?

Javascript to update endorsement button was lost when applying new design.

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG entry
  • Apply fix.

@ghost ghost assigned tramuntanal Mar 14, 2018
@ghost ghost added the in-progress label Mar 14, 2018
@mrcasals
Copy link
Contributor

@tramuntanal the i18n tests are failing, can you check them please?

@codecov
Copy link

codecov bot commented Mar 14, 2018

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2998      +/-   ##
==========================================
+ Coverage   98.67%   98.67%   +<.01%     
==========================================
  Files        1698     1699       +1     
  Lines       40500    40568      +68     
==========================================
+ Hits        39964    40032      +68     
  Misses        536      536

Copy link
Contributor

@mrcasals mrcasals left a comment

Choose a reason for hiding this comment

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

Code looks good to me! Can we have some system spec checking this? I'm pretty sure there's already some test checking the Endorse button works as expected, modify that spec the check that the button text is changed please 😄

@tramuntanal tramuntanal added the type: bug Issues that describe a bug label Mar 14, 2018

require "spec_helper"

describe "Endorse Proposal", type: :system do
Copy link
Contributor

Choose a reason for hiding this comment

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

So we had no system specs for this? 🤔 I think it's good we're adding them then! 😄

@mrcasals
Copy link
Contributor

@tramuntanal some tests fail, can you check them please? Also, you might need to rebase against master as we fixed a bug preventing the build_design_app job to work 😄

josepjaume
josepjaume previously approved these changes Mar 15, 2018
@@ -1,4 +1,10 @@
<% fully_endorsed= fully_endorsed?(proposal, current_user) %>
<%# main page button %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to be such a PITA over this, but we're not using comments like this anywhere in the project. We could start a discussion whether that's needed or it actually reflects a design flaw (I'm more inclined to that last argument), but for the moment, let's comply with the current style please :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Or this means this part could be extracted in a partial/cell 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it is actually a hint for a design flaw. I extracted the code into js methods.

@ghost ghost assigned josepjaume Mar 15, 2018
@josepjaume
Copy link
Contributor

josepjaume commented Mar 15, 2018

Hi! I've merged from master since the build wasn't passing due to a bug that's already fixed in there.

Oliver Valls added 2 commits March 16, 2018 09:08
…' of github.com:decidim/decidim into fix/2968-feedback_needed_after_endorsing_wo_user_groups
@tramuntanal
Copy link
Contributor Author

@decidim/lot-core It seems that last merge from master broke the specs. It seems a problem from master, can you check please?

@mrcasals
Copy link
Contributor

@tramuntanal rebasing against master will fix it!

@ghost ghost added the status: WIP label Mar 16, 2018
@josepjaume josepjaume merged commit 16f538b into master Mar 16, 2018
@ghost ghost removed the status: WIP label Mar 16, 2018
@josepjaume josepjaume deleted the fix/2968-feedback_needed_after_endorsing_wo_user_groups branch March 16, 2018 15:41
rbngzlv added a commit that referenced this pull request Mar 21, 2018
* master:
  [RFC] Use cells for meeting m cards (#3022)
  Do not force Postgresql user to be admin when enabling trigram extension (#3053)
  Make organization reference_prefix required (#3056)
  admin can duplicate/copy meetings (#3051)
  Fix question form errors not being displayed (#3046)
  Erb whitespace cutting (#3047)
  Show debates statistics on space show and homepage (#3016)
  Fix broken translated field after form errors (#3026)
  Move decidim executable to "exe" folder (#3028)
  Friendlier buttons (#3027)
  Feedback needed after Endorsing when user has no user_groups (#2998)
  Fix seeding error on generator specs (#3021)
  fix spelling error in threshold (#3019)
  Migration plus seeds (#2933)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review type: bug Issues that describe a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feedback needed after Endorsing
4 participants