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

Default stage extras to 'yes' for express courses #27888

Merged
merged 5 commits into from Apr 10, 2019

Conversation

maddiedierker
Copy link
Contributor

@maddiedierker maddiedierker commented Apr 4, 2019

LP-154

Sets stage extras to "yes" by default for express courses (was previously only defaulting to "yes" for courses 1-4 and a-f).

Note: Since we are now using the stage_extras_available set on scripts in levelbuilder, any script that has this property set to true will have stage extras enabled by default.

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Apologies for the scary red 🔴 👻but I think there is a better way.

script && /course[1-4a-f]/.test(script.script_name)
script &&
(/course[1-4a-f]/.test(script.script_name) ||
/express/.test(script.script_name))
Copy link
Member

Choose a reason for hiding this comment

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

This approach makes me sad and will be difficult to maintain. this info should really live as a property in the script. then when we create copies for new version years, this property will be copied over, i.e. if express-2018 has the flag, and you create express-2019 from it, then express-2019 would already have the flag.

Could we please take this opportunity to clean this up? The stage_extras_available property is already defined on many scripts, so all we would need to do is add it to the .script files for all the courses which currently match these regexes, and then remove this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, perfect! i hadn't known about the stage_extras_available property -- that's a much better way. i'll update and re-request review

@maddiedierker
Copy link
Contributor Author

@davidsbailey @dmcavoy i refactored this work with dave's suggestion, so please take a look!

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Thank you for doing this refactor!

Copy link
Contributor

@dmcavoy dmcavoy left a comment

Choose a reason for hiding this comment

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

Just want to check my understanding: After this goes in someone will need to go in and set stage_extras_available for the courses that we want this to default to yes? Also is it always true that if a script has stage extras available we would want it to default to yes? We use stage extras outside CSF.

@davidsbailey
Copy link
Member

Great catch Dani. Here are all the courses where we currently set it. I would definitely want to double check before enabling this for CSD:

Dave-MBP:~/src/cdo/dashboard/config/scripts$ grep -l stage_extras_available *.script | sort
allthethings.script
course1.script
course2.script
course3.script
coursea-2017.script
coursea-2018.script
coursea-2019.script
courseb-2017.script
courseb-2018.script
courseb-2019.script
coursec-2017.script
coursec-2018.script
coursec-2019.script
coursed-2017.script
coursed-2018.script
coursed-2019.script
coursee-2017.script
coursee-2018.script
coursee-2019.script
coursef-2017.script
coursef-2018.script
coursef-2019.script
csd3-2018.script
csd3-2019.script
csd6-2018.script
csd6-2019.script
csd6-draft.script
csd6-old.script
express-2017.script
express-2018.script
express-2019.script
pre-express-2017.script
pre-express-2018.script
pre-express-2019.script
time4csdemo.script

@davidsbailey
Copy link
Member

if stage extras needs to be disabled for any scripts, I'd suggest doing so in this PR rather than separately going into levelbuilder, to try and avoid any timing issues if this PR were to reach production first.

@maddiedierker
Copy link
Contributor Author

ooo, great point. i'll check with stakeholders to see if this list of scripts is okay to have stage extras turned on by default.

@dmcavoy re: your question...

Just want to check my understanding: After this goes in someone will need to go in and set stage_extras_available for the courses that we want this to default to yes?

yes, that's correct -- any script that should have stage extras enabled by default should set stage_extras_available in levelbuilder

@davidsbailey
Copy link
Member

Just want to check my understanding: After this goes in someone will need to go in and set stage_extras_available for the courses that we want this to default to yes?

yes, that's correct -- any script that should have stage extras enabled by default should set stage_extras_available in levelbuilder

FWIW, this setting should get copied over by default when cloning scripts, so e.g. coursea-2020 and similar will not need to have this set manually.

@maddiedierker
Copy link
Contributor Author

@mrjoshida confirmed that it's okay to enable stage extras by default for the above CSD scripts, so i think we're good to go

@maddiedierker maddiedierker merged commit eed3012 into staging Apr 10, 2019
@maddiedierker maddiedierker deleted the lesson-extras-default branch April 10, 2019 20:56
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