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

Export Assessments Score Summary in CSV Format #7640

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bivanalhar
Copy link
Contributor

Feature Description

Inside the Assessment Statistics (can be navigated through /courses/{:courseId}/statistics/assessments), user can pick several assessments in which they would like to get the score summary for all the students in the course. The resulting summary will be in CSV format

Screenshot 2024-11-15 at 2 59 37 PM

After done with selecting the assessments to be exported, then user can just navigate to the Download Button available on top of the table, and the final confirmation prompt will be shown to ensure that user has already chosen all assessments to be exported, as follows

Screenshot 2024-11-15 at 2 59 56 PM

The CSV produced will contain the following info, for each row:

  • Student's Name
  • Student's Email
  • Student's Status (Normal / Phantom)
  • The score obtained by this particular student for each assessments chosen

@bivanalhar bivanalhar force-pushed the bivan/assessments-score-summary-export branch from 650695a to ed35efa Compare November 15, 2024 07:10
Comment on lines 25 to 56
<div className="flex w-full justify-between">
{props.alternative.keepNative && props.onSearchKeywordChange && (
<SearchField
className="mr-4 lg:mr-0 lg:w-1/2"
onChangeKeyword={props.onSearchKeywordChange}
placeholder={props.searchPlaceholder ?? t(translations.search)}
value={props.searchKeyword}
/>
)}

<div className="flex items-center">
{props.alternative.render()}

{props.alternative.keepNative && (
<>
{props.buttons}

{props.onDownloadCsv && (
<Tooltip
title={
props.csvDownloadLabel ?? t(translations.downloadAsCsv)
}
>
<IconButton onClick={props.onDownloadCsv}>
<Download />
</IconButton>
</Tooltip>
)}
</>
)}
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This custom code looks like it should go into props.alternative.render() instead of touching the toolbar code itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, the implementation of this Table is that if the alternative is being introduced, and the condition suffices (i.e. some rows are getting selected), then this alternative will be rendered instead of the native one

In this particular situation, we want to keep whatever toolbar is initially rendered (i.e. Search bar and also Download CSV button) and hence I merely extend it by allowing user to indicate if they want to include the initial toolbar ones or not

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the wrong way to extend a library, there is too much duplicated code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no easy method to retain the search bar without modifying the library, then we can simply do without the search bar when assessments are selected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check failing rspec, one of the search fails, likely due to change in MUI table.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's because of the flaky test. I re-ran it and now it's all successful

config/locales/en/course/statistics.yml Outdated Show resolved Hide resolved
@bivanalhar bivanalhar force-pushed the bivan/assessments-score-summary-export branch 2 times, most recently from 18f2ae1 to 9282155 Compare November 18, 2024 09:21
protected

def perform_tracked(course, assessment_ids)
file_name = "#{Pathname.normalize_filename(course.title)}_score_summary_#{Time.now.strftime '%Y-%m-%d %H:%M'}.csv"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can just underscore the datetimestamp %Y%m%d_%H%M
otherwise we get a weirdly formatted filename like this: title of course_score_summary_2024-11-18 16_56.csv

Comment on lines 25 to 56
<div className="flex w-full justify-between">
{props.alternative.keepNative && props.onSearchKeywordChange && (
<SearchField
className="mr-4 lg:mr-0 lg:w-1/2"
onChangeKeyword={props.onSearchKeywordChange}
placeholder={props.searchPlaceholder ?? t(translations.search)}
value={props.searchKeyword}
/>
)}

<div className="flex items-center">
{props.alternative.render()}

{props.alternative.keepNative && (
<>
{props.buttons}

{props.onDownloadCsv && (
<Tooltip
title={
props.csvDownloadLabel ?? t(translations.downloadAsCsv)
}
>
<IconButton onClick={props.onDownloadCsv}>
<Download />
</IconButton>
</Tooltip>
)}
</>
)}
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the wrong way to extend a library, there is too much duplicated code.

@bivanalhar bivanalhar force-pushed the bivan/assessments-score-summary-export branch 2 times, most recently from 9918d83 to 1936a57 Compare November 20, 2024 03:44
@bivanalhar bivanalhar force-pushed the bivan/assessments-score-summary-export branch from 1936a57 to ff96f5b Compare November 20, 2024 04:54
@bivanalhar
Copy link
Contributor Author

bivanalhar commented Nov 20, 2024

This is the wrong way to extend a library, there is too much duplicated code.

@cysjonathan Updated to remove the duplication

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.

2 participants