codeeu / coding-events

A Django app for adding EU Code Week events and showing them on a map.
http://events.codeweek.eu
MIT License
17 stars 36 forks source link

Add active countries filter for scoreboard #419

Closed michelesr closed 8 years ago

michelesr commented 8 years ago

This patch should filter the entries in scoreboard to show only countries with numbers_of_event > 0 I got trouble in running the test in my local environment, so let's see if is all ok with Travis CI.

Your choice if merge or not this patch.

michelesr commented 8 years ago

Ok the test is broken because it expects to recieve a list of 250 countries

mitio commented 8 years ago

Okay. Are you planning to fix the test? If yes, I will wait for the fix before I push it live.

michelesr commented 8 years ago

Sure, i'll give it a check

michelesr commented 8 years ago

I close because i need to think better about the logic of the unit test (even if it passed now)... i'll reopen later.

michelesr commented 8 years ago

Well... the test will work if:

number or countries with at least an event == number of countries with at least an event starting in 2014 or later ...

this is coded here:

initial_counter = count_approved_events_for_country()
assert len(initial_counter) == len(list_active_countries())
michelesr commented 8 years ago

The filter can be rearranged to count only events from 2014 or later even in the scoreboard... let me know what you think about that.

Or even better... a good practice is to hardcode the expected result from the test instead of comparing the results of different method calls, so the assert instruction above could become:

assert len(initial_counter) == expected_count

Assuming that in the test db there aren't active countries, that will become:

assert len(initial_counter) == 0
michelesr commented 8 years ago

For now, i'll go for the hardcoding mode, which IMHO is better. (Tests shouldn't do computation but only verify predefined assertions)

mitio commented 8 years ago

Actually, the scoreboard should include only events from the current year and onward.

mitio commented 8 years ago

Also, I agree with your hardcoding preference in tests.

michelesr commented 8 years ago

Actually, the scoreboard should include only events from the current year and onward.

Ok, l'll update the filter then... wait before merging :-)

michelesr commented 8 years ago

Done.

Damn, those tabs instead of spaces in the code are very ugly, you should consider replacing them with 4 spaces as stated in PEP guidelines :-)

mitio commented 8 years ago

@michelesr So we're not filtering from the current year (e.g. 2015) onward yet?

As for the messy indentation--I completely agree, it should be unified. It bothers me as well. Are you aware of a tool to do that automatically with all files in the repo?

michelesr commented 8 years ago

Maybe you can open an issue for the indentation... i'll do a search for the tool.

The choice of 2014 as starting here come from Alessandro Bogliolo.

mitio commented 8 years ago

Okay, thanks. I'll discuss the year with him separately.

michelesr commented 8 years ago

:+1: