Improves the Stats Page

[panoramix360]

This PR improves the Stats Page #397

• Let the user person click on the Column title and change the ordering by ASC or DESC
• Add tests to the new code

[LuchoTurtle]

Thanks for opening the PR!
Before requesting review, please check if the tests are passing and if the coverage is at 100% 😄 . These are required for us to push PR/features into the main branch 😃 .

[panoramix360]

Yep! I'll add them.

I just opened the PR so you guys can take a look already, sorry for requesting the review, I should have waited haha

[panoramix360]

Finished all tasks!

Just needs testing now :) I'll try to finish the test cases this week.

[codecov]

Codecov Report

Merging #399 (43f34d3) into main (d919ce3) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #399   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        19    +4     
  Lines          490       525   +35     
=========================================
+ Hits           490       525   +35     

Files Changed	Coverage Δ
lib/app/item.ex	100.00% <ø> (ø)
lib/app/date_time_helper.ex	100.00% <100.00%> (ø)
lib/app/stats.ex	100.00% <100.00%> (ø)
lib/app_web/live/components/table_component.ex	100.00% <100.00%> (ø)
lib/app_web/live/stats_live.ex	100.00% <100.00%> (ø)
lib/app_web/views/table_component_view.ex	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[LuchoTurtle]

Looking awesome @panoramix360 , thank you kindly for the work!
Whenever you're ready, you can submit it for review 😍

[panoramix360]

hey @nelsonic and @LuchoTurtle, good Saturday 😄

I believe it's finished! I wrote the tests for the functions :D

The coverage is complaining about the sort_order column in the method person_with_item_and_timer_count in the item.ex file, but I don't really know why.

I tested the params so who knows haha if you guys have a hint, it would be helpful.

[panoramix360]

I was having some problems with the pipeline on Github.

in the test case Item.person_with_item_and_timer_count/0 returns a list of count of timers and items for each given person inside item_test.exs, I was testing the total timers but locally and on Github the timers give different results.

This attribute on the test case first_element.total_timers_in_seconds returns:

• Locally: Returns Decimal.new(0.000)
• Github: Returns 0.0

I don't know why this is happening, if you guys can help :)

[LuchoTurtle]

It looks great to me! 😍
We'll have to figure out why that specific line is not being covered.
Also, could you add these steps to the BUILDIT.md file? We are keeping track of each feature we push here, so it'd be great if you could give a small guide so other people would learn as well! Just add a new chapter and you can crack on (though I'd advise you to wait for @nelsonic 's review so you know it's ready :) )

[LuchoTurtle]

I get

Randomized with seed 65523
----------------
COV    FILE                                        LINES RELEVANT   MISSED
100.0% lib/api/item.ex                               218       56        0
100.0% lib/api/tag.ex                                101       24        0
100.0% lib/api/timer.ex                              152       40        0
100.0% lib/app/color.ex                               90        1        0
100.0% lib/app/date_time_helper.ex                    18        5        0
 98.4% lib/app/item.ex                               415       64        1
100.0% lib/app/item_tag.ex                            12        1        0
100.0% lib/app/tag.ex                                108       18        0
100.0% lib/app/timer.ex                              481       90        0
100.0% lib/app_web/controllers/auth_controller.       26        4        0
100.0% lib/app_web/controllers/init_controller.       41        6        0
100.0% lib/app_web/controllers/tag_controller.e       77       25        0
100.0% lib/app_web/live/app_live.ex                  441      130        0
100.0% lib/app_web/live/components/table_compon       11        1        0
100.0% lib/app_web/live/stats_live.ex                120       34        0
100.0% lib/app_web/router.ex                          49        9        0
100.0% lib/app_web/views/error_view.ex                59       12        0
  0.0% lib/app_web/views/profile_view.ex               3        0        0
100.0% lib/app_web/views/table_component_view.e       19        3        0
  0.0% lib/app_web/views/tag_view.ex                   3        0        0
[TOTAL]  99.8%

coverage. 🤔

[panoramix360]

I'll update the BUILDIT.md after @nelsonic review then :D

[panoramix360]

hey @nelsonic, I made some comments and fixed things as requested.

I'll wait for your help in the Ecto Query.

In regards to the docs, I created a PR on dwyl/book#96 to add the new section. Do you want me to remove it from here as well?

Thanks!

[nelsonic]

@panoramix360 yeah, the Ecto query syntax isn't great for these more complex queries ... 🙃

But not to worry. On further inspection of your validate_order/1 and validate_sort_column/1 functions,
they adequately mitigate the SQL injection attack in the person_with_item_and_timer_count/2 function
by invalidating common injection strings and defaulting to known (safe) values. 👌

Additionally:

• Added tests to ensure they stay safe. ✅
• Split the person_with_item_and_timer_count/2 function and your helpers out into lib/app/stats.ex 🆕
  so that it becomes easier to maintain/extend. 📈
• Split the stats related tests into test/app/stats_test.exs again for maintainability 💭
• Removed the additions from BUILDIT.md as already published in: dwyl.github.io/book/mvp/stats 📖
  thanks again for your PR Adds the Sorting Section inside the Stats Docs book#96 🎉

For anyone reading this in the future and curious about SQL injection mitigation, see: https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html 👀

[nelsonic]

@panoramix360 thanks again for this excellent addition. ❤️ ✅ 🚀

[nelsonic]

@LuchoTurtle feel free to merge once you've had a look.

[LuchoTurtle]

Looks great!
I'll just wait for @nelsonic 's reply to check if it's worth the effort to make get_person_id an unique function :)

[LuchoTurtle]

Alrighty! With that sorted, this is being merged!
Thanks @panoramix360 ❤️

[panoramix360]

yay! nice!

:D