Modernize instructor assessment statistics

[mylesw27]

This PR intends to convert the instructorAssessmentStatistics page from JavaScript and EJS to TypeScript and HTML template literals.

[github-actions]

All images

Image	Platform	Old Size	New Size	Change
prairielearn/executor:ef1fb35c994595fcec2473c2ccd56954d102e985	null	1648.27 MB	1648.95 MB	0.04%
prairielearn/prairielearn:ef1fb35c994595fcec2473c2ccd56954d102e985	linux/amd64	1648.26 MB	1648.94 MB	0.04%

[jonatanschroeder]

I feel strongly about avoiding patterns inside JS code. I'm ok with keeping the scripts inline for now, with a future PR moving it to an asset, but anything inside a script tag should be static.

I believe there is a case for combining the queries in this page to a single query, possibly two. Some of the queries involve the same tables, only with different columns. That said, again, I won't block on this, though I think this could be done in a follow-up PR.

[jonatanschroeder]

So much cleaner in an asset...

[jonatanschroeder]

Up to you if you want to do it here or in a future PR. histogram and parallel_histograms are in "localscript" files that could be very easily converted to asset libs, particularly since, AFAICT, they are only used in this page (unless there is any compatibility that needs to be considered for v2 questions). This would allow you to convert this declaration into imports, making the code cleaner. scatter is similar, but it's also used in instructorAssessmentQuestionStatistics, so you'd need to convert that page too. I'd be happy to approve this PR without it, but this is a pretty good opportunity to do this.

[mylesw27]

I added asset libs for histogram, parallel_histograms and scatter in 00ffe80. However, I have not removed the old localscript files until we receive confirmation on Slack that removing these will not have unintended consequences (like the v2 questions). I also have not made any updates to instructorAssessmentQuestionStatistics as it feels like we are starting to get out of the scope of this PR when we start touching that.

How would you feel if we left the old localscript files as is for now so we know that there are no breaking changes as far as v2 questions? I can then submit a follow up PR that would clean up things like removing the localscripts and making the update to instructorAssessmentQuestionStatistics. I also understand if we don't want to do this so we don't have dupes of histogram, parallel_histograms and scatter out there.

[jonatanschroeder]

I added asset libs for histogram, parallel_histograms and scatter in 00ffe80. However, I have not removed the old localscript files until we receive confirmation on Slack that removing these will not have unintended consequences (like the v2 questions). I also have not made any updates to instructorAssessmentQuestionStatistics as it feels like we are starting to get out of the scope of this PR when we start touching that.

That all make sense.

How would you feel if we left the old localscript files as is for now so we know that there are no breaking changes as far as v2 questions? I can then submit a follow up PR that would clean up things like removing the localscripts and making the update to instructorAssessmentQuestionStatistics. I also understand if we don't want to do this so we don't have dupes of histogram, parallel_histograms and scatter out there.

We already do this for some elements in public/javascripts, so it wouldn't be unprecedented (see #4658). However, it may be a good idea to add a comment at the top of these files indicating that they are being deprecated. Let's wait for a response from Matt or Nathan, though.

[nwalters512]

It should be safe to remove these files from localscripts. I checked course repos with GitHub code search and don't see any reference to those files in any questions. @mwest1066 what do you think, can we remove these or would you rather err on the side of caution and keep them around indefinitely?

[mwest1066]

I think that's a sufficient check to go ahead and remove them, given that we are out-of-semester for most courses.

[nwalters512]

👍 @mylesw27 want to go ahead and make this change?

[mylesw27]

Removed histogram, scatter and parallel_histograms from localscripts in 3896135. In order to be able to remove localscripts/scatter, I added a client script file for InstructorAssessmentQuestionStatistics so we can render the scatter there from assets/scripts/scatter.ts. This leaves the histmini on this page still using the localScript. Do you want me to move that in this PR or a subsequent PR?

[jonatanschroeder]

I added asset libs for histogram, parallel_histograms and scatter in 00ffe80. However, I have not removed the old localscript files until we receive confirmation on Slack that removing these will not have unintended consequences (like the v2 questions). I also have not made any updates to instructorAssessmentQuestionStatistics as it feels like we are starting to get out of the scope of this PR when we start touching that.

That all make sense.

How would you feel if we left the old localscript files as is for now so we know that there are no breaking changes as far as v2 questions? I can then submit a follow up PR that would clean up things like removing the localscripts and making the update to instructorAssessmentQuestionStatistics. I also understand if we don't want to do this so we don't have dupes of histogram, parallel_histograms and scatter out there.

We already do this for some elements in public/javascripts, so it wouldn't be unprecedented (see #4658). However, it may be a good idea to add a comment at the top of these files indicating that they are being deprecated. Let's wait for a response from Matt or Nathan, though.

[jonatanschroeder]

The global variable comment is a dealbreaker. Other comments are nits, though some of them also have applications in other parts of the code that I haven't commented.