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

Implement delivering client assets for custom elements in public question previews #9807

Open
wants to merge 10 commits into
base: sharing-element-client-code
Choose a base branch
from

Conversation

SethPoulsen
Copy link
Collaborator

partial replacement for #9776

resolves part of #9397

@SethPoulsen SethPoulsen marked this pull request as draft May 2, 2024 21:16
@SethPoulsen SethPoulsen changed the base branch from master to sharing-element-client-code May 2, 2024 21:16
Copy link
Contributor

github-actions bot commented May 2, 2024

All images

Image Platform Old Size New Size Change
prairielearn/executor:797e35992f78e545cf9eaddb7c54a67fd4b30ae6 null 1187.93 MB 1187.77 MB -0.01%
prairielearn/prairielearn:797e35992f78e545cf9eaddb7c54a67fd4b30ae6 linux/amd64 1187.91 MB 1187.77 MB -0.01%

@SethPoulsen SethPoulsen changed the title Implement delivering client assets for custom elements in publicly shared questions Implement delivering client assets for custom elements in public question previews May 3, 2024
@SethPoulsen
Copy link
Collaborator Author

note from meeting: add an index on the questions table on (course_id, shared_publicly) so we can quickly check if a course has any questions shared publicly.

Comment on lines 61 to 74
if (options.publicEndpoint && !options.static) {
const has_publicy_shared_question = await sqldb.queryRow(
sql.select_has_publicly_shared_question,
{ course_id: req.params.course_id },
z.boolean(),
);
if (!has_shared_question) {
if (!has_publicy_shared_question) {
throw new HttpStatusError(404, 'Not Found');
}
const course = await selectCourseById(req.params.course_id);
const coursePath = chunks.getRuntimeDirectoryForCourse(course);
await chunks.ensureChunksForCourseAsync(course.id, { type: 'elements' });

elementFilesDir = path.join(coursePath, 'elements');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the only actual new code here, the rest of the diff is muddled by the indentation level change.

@SethPoulsen SethPoulsen marked this pull request as ready for review May 27, 2024 18:21
@SethPoulsen
Copy link
Collaborator Author

This should go in after #9786

Copy link

codecov bot commented May 27, 2024

Codecov Report

Attention: Patch coverage is 74.00000% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 67.03%. Comparing base (19c4c84) to head (797e359).

Files Patch % Lines
...rairielearn/src/pages/elementFiles/elementFiles.js 79.10% 14 Missing ⚠️
apps/prairielearn/src/server.js 54.16% 0 Missing and 11 partials ⚠️
apps/prairielearn/src/lib/assets.ts 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                     Coverage Diff                      @@
##           sharing-element-client-code    #9807   +/-   ##
============================================================
  Coverage                        67.02%   67.03%           
============================================================
  Files                              458      458           
  Lines                            71457    71490   +33     
  Branches                          5733     5743   +10     
============================================================
+ Hits                             47896    47923   +27     
- Misses                           23126    23128    +2     
- Partials                           435      439    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +184 to +187
(await import('../pages/elementFiles/elementFiles.js')).default({
publicQuestionEndpoint: false,
static: true,
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unnecessary; why not import at the top-level scope like we had before? This isn't server.js where we want to avoid overloading the top of the file with imports.

if (!valid) {
throw new HttpStatusError(404, 'Unable to serve that file');
}
export default function (options = { publicQuestionEndpoint: false, static: false }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

JSDoc comment? I'm specifically wondering what static means here.

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.

None yet

3 participants