search

FAForever / downlords-faf-client

Official client for Forged Alliance Forever
https://faforever.com
MIT License
195 stars 121 forks source link

Fix leaderboard order #2988

Closed BlackYps closed 1 year ago

BlackYps commented 1 year ago

The 3v3 was listed last, because it has the highest id. So we have to switch to a different way to sort them, so they are in the order that you would expect. grafik

codecov[bot] commented 1 year ago

Codecov Report

Merging #2988 (783590b) into develop (329c775) will decrease coverage by 0.01%. The diff coverage is 100.00%.

:exclamation: Current head 783590b differs from pull request most recent head 37ca19b. Consider uploading reports for the commit 37ca19b to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #2988 +/- ## ============================================= - Coverage 61.33% 61.33% -0.01% Complexity 4594 4594 ============================================= Files 557 557 Lines 20185 20185 Branches 1048 1048 ============================================= - Hits 12381 12380 -1 Misses 7218 7218 - Partials 586 587 +1 ``` | [Impacted Files](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/2988?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever) | Coverage Δ | | |---|---|---| | [...ver/client/leaderboard/LeaderboardsController.java](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/2988?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever#diff-c3JjL21haW4vamF2YS9jb20vZmFmb3JldmVyL2NsaWVudC9sZWFkZXJib2FyZC9MZWFkZXJib2FyZHNDb250cm9sbGVyLmphdmE=) | `86.36% <100.00%> (ø)` | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/2988/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/2988?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/2988?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever). Last update [329c775...37ca19b](https://app.codecov.io/gh/FAForever/downlords-faf-client/pull/2988?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FAForever).
Sheikah45 commented 1 year ago

So we just switch to sorting by the name? Wouldn't this potentially have the same issue in the future? Maybe we should add an order parameter to the league object

BlackYps commented 1 year ago

I agree in principle, but adding an order parameter would be more than one order of magnitude more work. It would require changes to the league service, the commons repo and finally the client. All of this to fix a problem that we might have in the future. At the moment I don't know of any plans to add more leagues. So I propose that for now we roll with this change and if we notice that it actually causes problems in the future, then we can implement the more elaborate solution. Adding a new league would require changes to the league service anyway, so I could then easily add a new order parameter as well.