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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#12329] Refactoring of sortable tables - Instructor courses page #12537

Closed

Conversation

singhabhyudita
Copy link
Contributor

@singhabhyudita singhabhyudita commented Jul 30, 2023

Part of #12329

Outline of Solution
Refactored all the three tables ( Active Courses, Archived Courses and Deleted Courses ) into sortable tables. Created custom components for course stats in the active courses, and actions column in all the three tables.

Currently I need some help regarding how to change the corresponding spec.ts file to update it.

singhabhyudita and others added 25 commits June 16, 2023 16:13
Updating the changes in the forked repository
@domlimm
Copy link
Contributor

domlimm commented Jul 31, 2023

@singhabhyudita Please fix the conflicts and ensure the checks (the red Xs) are passing before we review. No hurry, thanks!

Currently I need some help regarding how to change the corresponding spec.ts file to update it.

Run npm run test and see if you can follow the options to update the specs.

@singhabhyudita
Copy link
Contributor Author

@singhabhyudita Please fix the conflicts and ensure the checks (the red Xs) are passing before we review. No hurry, thanks!

Currently I need some help regarding how to change the corresponding spec.ts file to update it.

Run npm run test and see if you can follow the options to update the specs.

I have already updates the snapshots. There are a few errors in the specs. Basically since I have moved the action buttons into another component, the button calls in the specs file are giving errors. Do I simply delete them?

@nusoss-bot
Copy link

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

@weiquu
Copy link
Contributor

weiquu commented Aug 12, 2023

I have already updates the snapshots. There are a few errors in the specs. Basically since I have moved the action buttons into another component, the button calls in the specs file are giving errors. Do I simply delete them?

Deleting the tests doesn't sit right with me... for example, disabling the enrol button when an instructor cannot modify students seems an important feature to test. I think we should still be testing functionality, but perhaps the tests could be shifted to different components. I suppose a way to keep the tests within instructor-courses-page.component.spec.ts is to traverse the DOM to look for the correct button, but then that's quite prone to breaking with any format changes, and also doesn't really fit with the purpose of spec tests (i.e. to test that particular component).

With the above in mind, I think that the way forward is to shift the spec tests into their relevant components. For example, the test for disabling the enrol button could be shifted to the cell-with-actions component. @singhabhyudita do let us know if this would require a significant amount of work - we can always have it as a separate PR.

I also took a look at the other failing spec tests and noted that the "should load all courses by the instructor" test is failing - I think this one should be fixed and left in the instructor-courses-page.component.spec.ts file.

@zhaojj2209 / @samuelfangjw / @wkurniawan07 - would appreciate any advice; am not too sure if shifting the tests into the newly-created components is the best approach, or if it might require too much setup.

@nusoss-bot
Copy link

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

@nusoss-bot
Copy link

Folks, This PR seems to be stalling (no activities for the past 8 days). 🐌 😢
Hope someone can get it to move forward again soon...

@nusoss-bot
Copy link

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

1 similar comment
@nusoss-bot
Copy link

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants