[#12467] Add Sort Functions to Admin Sessions Page

[singhabhyudita]

Fixes #12467

Outline of Solution

I tried using the sortable-table component. I have created a method populateSortableTable() which is called in the getFeedbackSessions() method. There are a few formatting changes that had to be done. The creator email Id is no longer a hyperlink and same goes for response rate. Previously the response rate was displayed by clicking on the show button, but due to the restrictions of the sortable-table component, I am unable to do so hence I have left it empty.

Any suggestions regarding how to display the response rate are welcome.

Also for the other two functions, one can be sort by institute name but I am not too sure about the other one.

[github-actions]

Hi @singhabhyudita, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

• Description must reference the issue number the PR is fixing, e.g. Fixes #<issue-number> (or Part of #<issue-number> if the PR does not address the issue fully)

Please address the above before we proceed to review your PR.

[singhabhyudita]

Hi @singhabhyudita, thank you for your interest in contributing to TEAMMATES! However, your PR does not appear to follow our contribution guidelines:

• Description must reference the issue number the PR is fixing, e.g. Fixes #<issue-number> (or Part of #<issue-number> if the PR does not address the issue fully)

Please address the above before we proceed to review your PR.

Fixed

[weiquu]

Hi @singhabhyudita, do fix the failing checks before we review your PR. You might also want to take a quick look at the changes to the sortable-table component in #12501 to see if it helps you. I'm not too sure if it'll affect your PR; my guess is no, but I'll need to take a closer look

[Zxun2]

Previously the response rate was displayed by clicking on the show button, but due to the restrictions of the sortable-table component, I am unable to do so hence I have left it empty.

I have managed to solve this issue in my PR (the same one that @weiquu linked to). You can have a look at it and see if it helps you :)

[singhabhyudita]

Sure I will take a look. Also there is one very trivial issue here, the sortable-table component is not working. I am trying to sort the tables by start / end time but it is unresponsive.

[singhabhyudita]

Current changes:

[singhabhyudita]

Pinging @weiquu @domlimm for review!

[weiquu]

Hi @singhabhyudita, a couple of comments to work on first:

Initially, the "Institution Name" button is disabled, indicating that the panels are sorted by institution name. However, that's not the case - clicking on "Institution Sessions Total" and back to "Institution Name" gives in a different sort order. Do ensure that the initial sort order is correct.

The sorting by start and end dates are also incorrect. Intuitively, the sort should be by a date comparison, so for example, ascending means that the dates should get later further down the rows. However, it seems that the current comparison is just based on strings, meaning that (for an ascending sort) Fridays will come first, then Mondays, etc, regardless of the actual date.

Will take a closer look at the code once these are fixed

[singhabhyudita]

Hi @singhabhyudita, a couple of comments to work on first:

Initially, the "Institution Name" button is disabled, indicating that the panels are sorted by institution name. However, that's not the case - clicking on "Institution Sessions Total" and back to "Institution Name" gives in a different sort order. Do ensure that the initial sort order is correct.

The sorting by start and end dates are also incorrect. Intuitively, the sort should be by a date comparison, so for example, ascending means that the dates should get later further down the rows. However, it seems that the current comparison is just based on strings, meaning that (for an ascending sort) Fridays will come first, then Mondays, etc, regardless of the actual date.

Will take a closer look at the code once these are fixed

For the start and end dates I just used the existing enums. On taking a closer look I found out that the SESSION_START_DATE and SESSION_END_DATE are being sorted lexicographically in the table-comparator service file.

[weiquu]

For the start and end dates I just used the existing enums. On taking a closer look I found out that the SESSION_START_DATE and SESSION_END_DATE are being sorted lexicographically in the table-comparator service file.

You're right that they are being sorted lexicographically; however, the underlying data is being stored as a number (timestamp) rather than the string. Hence, when sorted, the numbers are compared which produces the correct result. You can see this in action by going to the compareLexicographically method of the table-comparator.service.ts and logging strA and strB. When the admin sessions table is sorted, they output a date string; when say the sessions table in the instructor home page is sorted, they output a number representing the date.

My suggestion is to take a closer look at how the sessions table in the instructor home page is handled, including the createDateCellWithToolTip which you'll see transforms the timestamp (a number) into the string value to be displayed to the user. Some changes will probably be needed to be made to the OngoingSessionsModel (which I'm not sure why is defined in the response rate cell component... would have thought leaving it in the admin sessions page component would be better). I think now only the string is stored - either store both, or only store the endTime/startTime and then convert it to a string later on.

[singhabhyudita]

For the start and end dates I just used the existing enums. On taking a closer look I found out that the SESSION_START_DATE and SESSION_END_DATE are being sorted lexicographically in the table-comparator service file.

You're right that they are being sorted lexicographically; however, the underlying data is being stored as a number (timestamp) rather than the string. Hence, when sorted, the numbers are compared which produces the correct result. You can see this in action by going to the compareLexicographically method of the table-comparator.service.ts and logging strA and strB. When the admin sessions table is sorted, they output a date string; when say the sessions table in the instructor home page is sorted, they output a number representing the date.

My suggestion is to take a closer look at how the sessions table in the instructor home page is handled, including the createDateCellWithToolTip which you'll see transforms the timestamp (a number) into the string value to be displayed to the user. Some changes will probably be needed to be made to the OngoingSessionsModel (which I'm not sure why is defined in the response rate cell component... would have thought leaving it in the admin sessions page component would be better). I think now only the string is stored - either store both, or only store the endTime/startTime and then convert it to a string later on.

Can we maybe create a function that converts the dates into a Date object and then sorts it?

I will also look at the instructor home page sessions table.
Also the OngoingSessionsModel has been shifted to cellWithResponseRateComponent because there was a dependency cycle being created since the ongoing sessions object was being passed into the response rate component.

[weiquu]

Can we maybe create a function that converts the dates into a Date object and then sorts it?

Don't see any issue with that, though I'm not sure if it's easier or cleaner than the current approach. (Do note that, although unlikely, there might be an issue with this that might be uncovered later.)

Also the OngoingSessionsModel has been shifted to cellWithResponseRateComponent because there was a dependency cycle being created since the ongoing sessions object was being passed into the response rate component.

Hmm haven't taken a close look at the code, but is there any difference with the cell-with-response-rate.component.ts being used in the sessions-table component? For quick reference, here are the inputs being used there:

  @Input() responseRate: string = '';
  @Input() idx: number = 0;
  @Input() empty: boolean = false;
  @Input() isLoading: boolean = false;
  @Input() onClick: () => void = () => { };

My intuition is that the implementation of this should be the same, not too sure why the ongoing sessions object is being passed in. Do let me know if I've missed something (and feel free to modify or add to the fields in the ongoing sessions object).

[singhabhyudita]

The implementation here is different because the in the other case the changes are being made to the sessions table component (which is using the sortable-table component) , which is in turn being used in the instructor sessions page. In this case the admin sessions page is directly using the sortable-table component. Probably that's why createCellWithResponseRateComponent has a different implementations.

I have fixed the sorting issue by converting the date into milliseconds. Also fixed the initial sort issue.

[singhabhyudita]

Pinging @weiquu @domlimm for review

[singhabhyudita]

Pinging @weiquu @domlimm for review

@weiquu @domlimm @zhaojj2209

[weiquu]

Hi @singhabhyudita, apologies for the delay - been rather busy. Thanks for the changes to the sorting by start and end date within tables, those work fine now. Before we take a closer look at the actual implementation, could you help to fix the following issues?

1. Sort button does not reset

I first sort the panels by "Institution Sessions Total", which gives the following result:

Afterwards, I increase the date range and click "Filter by Range". This gives the following result:

As can be seen, even though the "Institution Sessions Total" button is still shown to be selected, the panels are not sorted accordingly. I think that either the Sort By should be sorted by whatever is currently selected, or it should reset to "Institution Name" (and the button should also reset). Both approaches are fine.

Further note that the panels are also not correctly sorted by Institution Name ("test" should be after "TEAMMATES Test Institute 1"). This is likely related to the next issue:

2. Issue with initial sort by institution name

After setting a date range and pressing "Filter by Range", I get the following sort:

However, the sort above is incorrect. After pressing "Institution Sessions Total" and then back to "Institution Name", I get the following correct sort:

The sort should be correct even upon the initial loading.

3. Panels not closing properly

I first set the date range and press "Filter by Range". Then, I sort by "Institution Sessions Total". Afterwards, I try to close the first panel; however, the panel does not close, even though the icon shows that it is closed. Upon pressing it again (to open the already open panel), I get what appears to be 2 tables under 1 panel:

Furthermore, after trying to close the panel, one of the tables disappears, while one remains:

Note that the icon shows that the panel is closed, yet there is a table still visible beneath it.

Do let me know if you have any trouble recreating these issues.

[nusoss-bot]

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

[cedricongjh]

hey @singhabhyudita, we will be closing this PR for now, if you're working on it still, do re-open it!

[singhabhyudita]

Hi @cedricongjh , have been occupied for a while. Will start working on it again by the end of this week. Thanks.

[singhabhyudita]

Hi @weiquu @cedricongjh can we reopen this PR. I have started working on it again.

[singhabhyudita]

Hi @singhabhyudita, apologies for the delay - been rather busy. Thanks for the changes to the sorting by start and end date within tables, those work fine now. Before we take a closer look at the actual implementation, could you help to fix the following issues?

1. Sort button does not reset

I first sort the panels by "Institution Sessions Total", which gives the following result:

Afterwards, I increase the date range and click "Filter by Range". This gives the following result:

As can be seen, even though the "Institution Sessions Total" button is still shown to be selected, the panels are not sorted accordingly. I think that either the Sort By should be sorted by whatever is currently selected, or it should reset to "Institution Name" (and the button should also reset). Both approaches are fine.

Further note that the panels are also not correctly sorted by Institution Name ("test" should be after "TEAMMATES Test Institute 1"). This is likely related to the next issue:

2. Issue with initial sort by institution name

After setting a date range and pressing "Filter by Range", I get the following sort:

However, the sort above is incorrect. After pressing "Institution Sessions Total" and then back to "Institution Name", I get the following correct sort:

The sort should be correct even upon the initial loading.

3. Panels not closing properly

I first set the date range and press "Filter by Range". Then, I sort by "Institution Sessions Total". Afterwards, I try to close the first panel; however, the panel does not close, even though the icon shows that it is closed. Upon pressing it again (to open the already open panel), I get what appears to be 2 tables under 1 panel:

Furthermore, after trying to close the panel, one of the tables disappears, while one remains:

Note that the icon shows that the panel is closed, yet there is a table still visible beneath it.

Do let me know if you have any trouble recreating these issues.

@weiquu thank you for pointing this out. I have fixed the first 2 issues.
The panel issue however is something weird. When I try to close the first few tables the issue exists but not for the last table.

When I switch the sorting option while the tables are expanded and then try to close the tables the issue exists. But when the table is collapsed and I switch it works fine. I am not able to understand where the problem lies exactly because it is not consistent among all tables.