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

Fix registrations displaying #5599

Merged
merged 1 commit into from Jan 30, 2015

Conversation

@ghost
Copy link

commented Jan 27, 2015

This should fix the registration problem #5589

@@ -4,13 +4,21 @@

module StatisticsHelper
def registrations_status statistics
if statistics.open_registrations?
if !!statistics.open_registrations?

This comment has been minimized.

Copy link
@dimaursu

dimaursu Jan 29, 2015

Contributor

um? That's double negation, it's the same as the original.

This comment has been minimized.

Copy link
@ghost

ghost Jan 29, 2015

Author

Yeah, problem is : function recieve a symbol, so, as is is not a fasly value, it is always considered true, even if registrations are closed. !! "operator" forces cast to boolean. Works well ;)

This comment has been minimized.

Copy link
@jhass

jhass Jan 29, 2015

Member
  • If you can call open_registrations? on statistics, that's certainly not a symbol.
  • If open_registrations? returns a symbol, !!symbol is always true.
  • if open_registrations? returns nil or a symbol, nil is falsey and the symbol is truthy, so the !! is unneeded.

This comment has been minimized.

Copy link
@ghost

ghost Jan 29, 2015

Author

Well I can't explain but I can assure you that it works with and doesn't work without :/

This comment has been minimized.

Copy link
@jhass

jhass Jan 29, 2015

Member

Add require "pry"; binding.pry in the line before that and trigger the code path.

This comment has been minimized.

Copy link
@jhass

jhass Jan 29, 2015

Member

Or just add a ? to https://github.com/diaspora/diaspora/blob/develop/app/presenters/statistics_presenter.rb#L34 to properly fix it.

Also add an RSpec test that triggers the bug, verify it's failing without the fix.

I18n.t('statistics.open')
else
I18n.t('statistics.closed')
end
end

def registrations_status_class statistics
if !!statistics.open_registrations?

This comment has been minimized.

Copy link
@dimaursu

dimaursu Jan 29, 2015

Contributor

Same as before, double negation

@@ -31,7 +31,7 @@ def version
end

def open_registrations?
AppConfig.settings.enable_registrations
AppConfig.settings.enable_registration?

This comment has been minimized.

Copy link
@ghost

ghost Jan 30, 2015

Author

@jhass : Nope, see by yourself. It doesn't work, plus it makes to other tests to fail. :/

This comment has been minimized.

Copy link
@jhass

jhass Jan 30, 2015

Member

Take a close look at the line you commented on.

This comment has been minimized.

Copy link
@ghost

ghost Jan 30, 2015

Author

Ok, are you willing to continue asking me riddles or will you eventually help me ?
You told me tu put a ? at the end of that line and I did and it doesn't work. So I'm sorry if I'm missing something but right now, I've got better to do than playing puzzles.

This comment has been minimized.

Copy link
@jhass

jhass Jan 30, 2015

Member

You didn't add a ?, you replaced the s with it.

This comment has been minimized.

Copy link
@ghost

ghost Jan 30, 2015

Author

Hmmf... You are right, sorry.

@ghost

This comment has been minimized.

Copy link
Author

commented Jan 30, 2015

Ok, I think it can be merged now.

@jhass jhass added this to the next-major milestone Jan 30, 2015

jhass added a commit that referenced this pull request Jan 30, 2015

@jhass jhass merged commit 9bfabe6 into diaspora:develop Jan 30, 2015

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
@jhass

This comment has been minimized.

Copy link
Member

commented Jan 30, 2015

Thanks!

@ghost

This comment has been minimized.

Copy link
Author

commented Jan 30, 2015

Thanks to you.

@ghost ghost deleted the fix-stats-page-regristration branch Feb 14, 2015

@ghost ghost restored the fix-stats-page-regristration branch Mar 11, 2015

@ghost ghost deleted the fix-stats-page-regristration branch Mar 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.