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

Create the rankings page on rails! #4394

Merged
merged 1 commit into from Aug 13, 2019

Conversation

@AlbertoPdRF
Copy link
Member

commented Aug 2, 2019

EDIT: it should be ready to go! 馃殺

I think I've come a long way already, but there are some things that are preventing me to finish with this and I figured out it would be better to discuss them while others are able to see the code.

Problems/questions I'm having:

  1. I can't seem to get the JavaScript for the region selector to work 馃槥 I found the request.params.merge(param: param) to work quite well for the other selectors, but I don't know if that's what I should be using.
  2. How can I test if what I wrote to support the old links work? I'm pretty sure it doesn't though 馃槢
  3. Are more tests necessary? If yes, of what kind?
  4. This is quite nit-picky but... how should the SQL queries be indented?

Besides, I still have to find a clever way of crafting the "By Region" table. I might come up with more questions regarding this later.

I'm not sure if I'm forgetting something, but with the problems/questions above and the code itself I think there's enough to start a conversation! 馃槅

@AlbertoPdRF AlbertoPdRF added the wip label Aug 2, 2019

@AlbertoPdRF AlbertoPdRF force-pushed the AlbertoPdRF:PortRankingsToRails branch from 077848f to 2869ae4 Aug 5, 2019

@Mollerz

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Something to add that I remember seeing an issue before but I can no longer find... It would be good to show WR, OcR, AfR, SAR, NAR, ER, AsR, or NR next to the time in the rankings list to show how the time stacks up against the rest of the world.

Another thing that might be nice, I noticed on your first image you posted you had the flag of the person to show which country they are from. It might be a fun idea to also include the flag of the country next to the competition to show where it was held as well! Especially if it shows WCA World Championship or a local city name, it is hardly ever obvious where the competition is held.

@AlbertoPdRF

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

Something to add that I remember seeing an issue before but I can no longer find... It would be good to show WR, OcR, AfR, SAR, NAR, ER, AsR, or NR next to the time in the rankings list to show how the time stacks up against the rest of the world.

I would rather leave this for later, unless it's really trivial and people see it as necessary, and just focus on porting everything "as is." There are other suggested additions to the tables (some are on #301 itself,) and probably there's a better way to compute some of the tables (at least for some given combinations of parameters) than just porting and slightly modifying the SQL queries from the PHP code, but once we have everything in RoR we can better think how to improve it.

Another thing that might be nice, I noticed on your first image you posted you had the flag of the person to show which country they are from. It might be a fun idea to also include the flag of the country next to the competition to show where it was held as well! Especially if it shows WCA World Championship or a local city name, it is hardly ever obvious where the competition is held.

See:

image

I'm fine keeping this, unless people feel like it looks a bit too overloaded of flags.

@Mollerz

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

Fair point on the record marking. I'll leave it up to others thoughts on the flags, personally I like it!

@AlbertoPdRF AlbertoPdRF force-pushed the AlbertoPdRF:PortRankingsToRails branch from b8491cd to f3aa691 Aug 6, 2019

@viroulep

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@tussosedan I know you expressed interest in porting stuff to rails, so maybe you'd like to check this out or help review this ;)

@tussosedan

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@viroulep definitely interested! I'm not there yet though, need to find time to start with the basics :) It's a bit crazy now with the kids' summer vacation, but I will get to it for sure.

@jonatanklosko
Copy link
Member

left a comment

Thanks for working on that, it's some tricky stuff ^^ Some comments from me 馃殌

Show resolved Hide resolved WcaOnRails/app/assets/stylesheets/results.scss Outdated
Show resolved Hide resolved WcaOnRails/app/assets/stylesheets/wca.scss Outdated
Show resolved Hide resolved WcaOnRails/app/helpers/results_helper.rb Outdated
Show resolved Hide resolved WcaOnRails/app/controllers/results_controller.rb
Show resolved Hide resolved chef/site-cookbooks/wca/templates/worldcubeassociation.org.conf.erb Outdated
Show resolved Hide resolved WcaOnRails/app/views/results/_index_table.html.erb Outdated
Show resolved Hide resolved WcaOnRails/app/views/results/_index_by_region_table.html.erb Outdated
Show resolved Hide resolved WcaOnRails/app/views/results/_index_by_region_table.html.erb Outdated
Show resolved Hide resolved WcaOnRails/app/controllers/results_controller.rb Outdated
Show resolved Hide resolved WcaOnRails/app/controllers/results_controller.rb Outdated

@AlbertoPdRF AlbertoPdRF force-pushed the AlbertoPdRF:PortRankingsToRails branch 3 times, most recently from 580afab to 07e5567 Aug 8, 2019

@AlbertoPdRF

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

After quickly chatting with @jfly, I pushed 07e5567 to test the code on staging. With that change, I could try and see that everything works as expected. I didn't leave this deployed to staging though, given that #4394 (comment) still needs to be taken care of.

@viroulep

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

There are a lot of plain SQL in the PR currently (ported from the php obviously :)), and just like Lars and Tim mentioned in previous "results work" here, I think we should make some effort to get what we want through ActiveRecord.

Note: I did not actually look into what the queries were doing, so maybe we have to write them manually.

Good luck getting everything to work @AlbertoPdRF, that's definitely a big task you're taking on!

@AlbertoPdRF AlbertoPdRF marked this pull request as ready for review Aug 9, 2019

@AlbertoPdRF

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

I actually started working on this by trying to get things through ActiveRecord, but after finding some difficulties and chatting with @jfly, he recommended me to use the queries directly and said that we could try to improve things when the page is ported.

Good luck getting everything to work @AlbertoPdRF, that's definitely a big task you're taking on!

Thanks 馃樃

@AlbertoPdRF AlbertoPdRF force-pushed the AlbertoPdRF:PortRankingsToRails branch from 65e092d to cb56b57 Aug 10, 2019

@AlbertoPdRF AlbertoPdRF removed the wip label Aug 10, 2019

@AlbertoPdRF AlbertoPdRF changed the title Port the rankings page to rails! Create the rankings page on rails! Aug 10, 2019

@AlbertoPdRF

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2019

I think I've addressed all the concerns. After chatting with @jfly, we have decided that this PR should be merged using the new-results URL and without removing the old PHP code so everything can be tested on the real production server. Another PR will follow with the changes that I dropped from this one to make the switch.

@AlbertoPdRF AlbertoPdRF force-pushed the AlbertoPdRF:PortRankingsToRails branch from cb56b57 to ff58730 Aug 11, 2019

Create the rankings page on rails!
Huge step towards #301.

@AlbertoPdRF AlbertoPdRF force-pushed the AlbertoPdRF:PortRankingsToRails branch from ff58730 to c187de1 Aug 12, 2019

@jfly

jfly approved these changes Aug 13, 2019

Copy link
Member

left a comment

I only skimmed this very quickly, but LGTM! Awesome work!

@jonatanklosko
Copy link
Member

left a comment

LGTM, thanks for taking another porting step!

@AlbertoPdRF

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Thanks guys for all the help! I'm really excited for this to go live 馃槃

@AlbertoPdRF AlbertoPdRF merged commit 81d852d into thewca:master Aug 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@AlbertoPdRF AlbertoPdRF deleted the AlbertoPdRF:PortRankingsToRails branch Aug 13, 2019

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