freeCodeCamp / classroom

BSD 3-Clause "New" or "Revised" License
144 stars 121 forks source link

Fixing the broken dashboard and breaking up logic in more readable fi… #402

Closed ngillux closed 1 year ago

ngillux commented 1 year ago

…les.

Checklist:

Closes #386

lloydchang commented 1 year ago

Hi @utsab and @ngillux

Thanks for the clarification in https://github.com/freeCodeCamp/classroom/pull/414#discussion_r1279892351

You may have noticed this already, but if not yet — Please note that CI is currently broken in PR #402

https://github.com/freeCodeCamp/classroom/actions/runs/5707369908/job/15464008361?pr=402

Run npm run test
  npm run test
  shell: /usr/bin/bash -e {0}

> @freecodecamp/classroom@0.0.0 test
> jest .

FAIL __tests__/__util__/processDashboardData.test.js
  ● processDashboardData › returns correct data structure

    TypeError: (0 , _processDashboardData.processDashboardData) is not a function

      8[2](https://github.com/freeCodeCamp/classroom/actions/runs/5707369908/job/15464008361?pr=402#step:6:2) |
      8[3](https://github.com/freeCodeCamp/classroom/actions/runs/5707369908/job/15464008361?pr=402#step:6:3) |     // Call the processDashboardData function with mockProps
    > 8[4](https://github.com/freeCodeCamp/classroom/actions/runs/5707369908/job/15464008361?pr=402#step:6:5) |     const result = processDashboardData(mockProps);
         |                                        ^
      8[5](https://github.com/freeCodeCamp/classroom/actions/runs/5707369908/job/15464008361?pr=402#step:6:6) |
      8[6](https://github.com/freeCodeCamp/classroom/actions/runs/5707369908/job/15464008361?pr=402#step:6:7) |     // Check that result is an object with 'data' and 'columns' properties
      8[7](https://github.com/freeCodeCamp/classroom/actions/runs/5707369908/job/15464008361?pr=402#step:6:8) |     expect(result).toHaveProperty('data');

      at Object.<anonymous> (__tests__/__util__/processDashboardData.test.js:[8](https://github.com/freeCodeCamp/classroom/actions/runs/5707369908/job/15464008361?pr=402#step:6:9)4:40)

PASS __tests__/components/authButton.test.jsx
PASS __tests__/components/dashtable_v2.test.jsx
PASS __tests__/components/navbar.test.jsx

Test Suites: 1 failed, 3 passed, 4 total
Tests:       1 failed, 4 passed, 5 total
Snapshots:   2 passed, 2 total
Time:        1.87 s
Ran all test suites matching /./i.
Error: Process completed with exit code 1.
Screen Shot 2023-07-31 at 8 45 05 PM Screen Shot 2023-07-31 at 8 43 24 PM
ngillux commented 1 year ago

Hi @lloydchang, the test that is failing was added by your mentees that incorporated the old JSON schema and old dashboard logic. I didn't remove that test as they could potentially update it to match the new JSON schema and the correct dashboard logic which uses the new JSON schema.

TLDR:

lloydchang commented 1 year ago

Hi @ngillux

There appears to be confusion.

@theGaryLarson and @Komal914 are @sijin-raj's mentees, and they aren't my mentees.

Hence I defer to @theGaryLarson, @Komal914, @sijin-raj to resolve https://github.com/freeCodeCamp/classroom/pull/402#issuecomment-1659525429 with you.

Thank you!

———

Via CodeDay Slack, I just asked @sijin-raj @theGaryLarson @Komal914 to resolve this with you.

@sijin-raj @theGaryLarson @Komal914

Would you resolve https://github.com/freeCodeCamp/classroom/pull/402#issuecomment-1659525429 and https://github.com/freeCodeCamp/classroom/pull/402#issuecomment-1659565162 with @ngillux?

Thank you!

https://codedayorg.slack.com/archives/C05E350DQH4/p1690865748250259

Komal914 commented 1 year ago

Hi @ngillux @lloydchang, @theGaryLarson and I can investigate why this merge is causing the test to fail.

ngillux commented 1 year ago

@lloydchang thank you for clarifying.

Hi @Komal914 and @theGaryLarson, thank you for trying to investigate, however this would be redundant; we are already aware why this issue is happening so no need to investigate. To summarize: the test that was written in #353 ( tests/util/processDashboardData.test.js) was written to accommodate the old JSON schema.

I purposely didn't remove it or update so you and Gary can work on it as a separate issue, and modify to accommodate the new JSON. My idea was to merge this to immediately address the broken state of the dashboard that the mentioned PR effected. I defer to @GuillermoFloresV or @utsab to determine how to proceed.

ngillux commented 1 year ago

Hi @utsab and @GuillermoFloresV I removed the redundant test 'util/processDashboardData' since a test was added in #366 that checked for the correct data being passed in from the props.

utsab commented 1 year ago

This PR introduced an odd bug. A simpler fix was introduced in PR #421.