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

[Poetry] Support example solutions #43667

Merged
merged 9 commits into from
Nov 18, 2021
Merged

Conversation

maddiedierker
Copy link
Contributor

@maddiedierker maddiedierker commented Nov 17, 2021

This creates support for example solutions in two ways:

  1. Renders the "Example Solutions" partial on the levelbuilder edit page for Poetry levels
  2. Adds support for Poetry levels in ScriptLevel#get_example_solutions so that the generated URLs for Poetry example solutions are correct. (By default, they were generating /projects/spritelab URLs.)

Relevant parts of the levelbuilder edit page:
Screen Shot 2021-11-18 at 10 27 35 AM

There is a necessary dependency between a Poetry level's standalone_app_name (called "Level Type" in the levelbuilder UI) and its examples. This is because the two standalone apps have different standalone project URLs (/projects/poetry and /projects/poetry_hoc) and examples are an array of channel tokens. This means if you change the standalone_app_name without updating that level's examples, all example solutions will be in a weird state because the project will load as a 'poetry' project rather than a 'poetry_hoc' project, and vice versa.

Javalab is a special case where examples are an array of URLs, which I think is the ideal setup for all level examples and would prevent broken URLs (although the examples would point to a different flavor of Poetry level). However, it seemed confusing to have Poetry levels match this special case since no other GamelabJr levels have that behavior.

Links

@maddiedierker maddiedierker requested review from a team, dmcavoy and mikeharv November 17, 2021 21:49
:ruby
warnings = {
level_type: "Example solutions are dependent on the level type field for generating solution URLs. If you change the level type, any existing example solution URLs will break.",
encrypted_examples: "For Poetry levels, the project URL for example solutions depends on the level type. Any existing example solutions will break if you change the level type without updating the channel tokens here."
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we clarify this warning when we say "depends on level type"? I want it to be clear that we're just talking about "Poetry" vs "Poetry HOC" and not about somehow changing from Poetry to a level type with a different lab.

Ugh, I think the issue is that we suddenly have two meanings for "level type". I think most writers interpret "level type" as things like Poetry/Spritelab/Applab/Video/etc, but in Poetry levels, it also means one of the two modes, "Poetry" (module) and "Poetry HOC". I think I'd go so far as to say that "level type" is the wrong description for changing between these two modes. I'd like for us to look at renaming this setting later, but also clarify the warning for writers now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, agreed that that's really unclear. in the code we refer to this field as standalone_app_name -- it's only labeled as "Level Type" in this UI. would some formulation of the actual field be clearer? something like "poetry standalone app type" or "poetry app type"?

this won't affect any code so i think we can go with whatever is clearest for the UI here

Copy link
Contributor

Choose a reason for hiding this comment

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

poetry level subtype?

Copy link
Contributor

Choose a reason for hiding this comment

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

"Poetry App Type" or "Poetry Level Subtype" work for me. Could we update the warning to include the two possible options like this?

For Poetry levels, the project URL for example solutions depends on the level subtype (Poetry or Poetry HOC).

I think this clarification would help for levelbuilders who are unfamiliar with this new term.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in this commit! (screenshot in PR description is updated too so you can see how it all looks together)

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks - lgtm!

Copy link
Contributor

@mikeharv mikeharv left a comment

Choose a reason for hiding this comment

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

Pulled and tested. Approved on the condition that we clarify the warning text.

@dmcavoy
Copy link
Contributor

dmcavoy commented Nov 18, 2021

Would there be a way on save to check that the example solutions match the level type and raise an error if they do not?

@mikeharv
Copy link
Contributor

@dmcavoy

Would there be a way on save to check that the example solutions match the level type and raise an error if they do not?

I don't think so. The solution provided is just the token that can be used with either type. It doesn't break, it's just that it displays in the wrong mode. I don't think this is a major issue. Any curriculum writer would logically want to make sure they're using the right type and would hopefully already be checking that solutions are still accurate after making major changes to the level. Speaking from experience, setting this type is also one of the first things I do when designing a new Poetry level.

@maddiedierker maddiedierker merged commit 9dd8a7f into staging Nov 18, 2021
@maddiedierker maddiedierker deleted the example-solutions-poetry branch November 18, 2021 20:27
snickell pushed a commit that referenced this pull request Feb 3, 2024
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

4 participants