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

Add UI to clone lessons to another script #40599

Merged
merged 11 commits into from May 21, 2021
Merged

Conversation

bethanyaconnor
Copy link
Contributor

@bethanyaconnor bethanyaconnor commented May 14, 2021

Finishes PLAT-963. Adds a button to the lesson tokens to clone the level to another script.

Successful clone:

Screen.Recording.2021-05-14.at.11.24.00.AM.mov

Clone with errors:

Screen.Recording.2021-05-14.at.11.19.18.AM.mov

We had talked about adding script name autocomplete. I didn't add that right now as they would've complicated this already sizeable PR, it's usable as is, and there is a decent error message if the user misspells the script name. Definitely possible as a future improvement if people think it's worth it.

Testing story

lot of manual testing, unit tests

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@bethanyaconnor bethanyaconnor changed the title Lesson copy UI Add UI to clone lessons to another script May 14, 2021
@bethanyaconnor bethanyaconnor marked this pull request as ready for review May 17, 2021 14:24
@bethanyaconnor bethanyaconnor requested review from tess323 and a team May 17, 2021 14:24
@tess323
Copy link

tess323 commented May 17, 2021

LGTM - agreed we can wait for feedback on the autocomplete. Thank you for all of your hard work on this!

return fetch(`/lessons/${this.props.lessonId}/clone`, {
method: 'POST',
body: JSON.stringify({
destinationScriptName: this.state.destinationScript
Copy link
Contributor

Choose a reason for hiding this comment

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

I have renaming on the brain and it reminded me that we are trying to move from Script to Unit. Should we try to use Unit here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Is the script to unit change a known move? Wondering if/how I should update the text on this dialog -- maybe something like "Which script/unit do you want to clone this lesson to?"

Copy link
Contributor

Choose a reason for hiding this comment

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

It's part of our Q2 plan - https://codedotorg.atlassian.net/browse/PLAT-791. I don't know how deep of a dive we were planning to try to do to move away from script

@dmcavoy
Copy link
Contributor

dmcavoy commented May 18, 2021

I pulled this locally to test how it worked to move things from aiml-2021 to csd7-2021. I ran into an issue when a unit has no lesson groups.

This line in def self.copy_to_script(original_lesson, destination_script)

    copied_lesson.lesson_group_id = destination_script.lesson_groups.last.id

Ends up not finding a lesson group if the script hasn't had one added to it. It might be worth adding something to create a lesson group that is not user-facing if one does not exist? Or giving a warning that lets the user know they should go make one. The error I got was undefined method id' for nil:NilClass`

@dmcavoy
Copy link
Contributor

dmcavoy commented May 18, 2021

Also looks like you can't go to a unit and save a lesson group with nothing in it. https://codedotorg.atlassian.net/browse/PLAT-1065

Worked great once I added a lesson so it forced it to have a lesson group

@dmcavoy
Copy link
Contributor

dmcavoy commented May 18, 2021

Love how simple the UI for this ending up being!

@bethanyaconnor
Copy link
Contributor Author

Ends up not finding a lesson group if the script hasn't had one added to it. It might be worth adding something to create a lesson group that is not user-facing if one does not exist?

oh good call. I noticed this when adding unit tests but that was before the empty script discussion. Should be easy to add a level group if one doesn't exist. If it's not, I'll do it in a follow-up PR

@bethanyaconnor
Copy link
Contributor Author

Should be easy to add a level group if one doesn't exist. If it's not, I'll do it in a follow-up PR

I managed to break seeding on my local environment trying to test adding a lesson group, so I'm just going to throw an error and then have a better fix in a follow-up

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.

Nice improvements! Excited to see this go live!

@bethanyaconnor bethanyaconnor merged commit fcb4d5f into staging May 21, 2021
@bethanyaconnor bethanyaconnor deleted the lesson-copy-ui branch May 21, 2021 16:50
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