feat: custom instruction and hint UI

[evemartin]

This change is 

[faucomte97]

Reviewed 2 of 15 files at r1, 5 of 13 files at r2, all commit messages.
Reviewable status: 7 of 16 files reviewed, 12 unresolved discussions (waiting on @evemartin)

---

game/messages.py line 104 at r2 (raw file):

def title_level_default():

This isn't used anywhere - please remove (I know you didn't work on this but it ties in closely with what this PR is doing)

---

game/messages.py line 108 at r2 (raw file):

def description_level_default():

This is the default description/lesson text, if you specify it in models you should be able to remove it from here. Be mindful of where this current function is being used though.

---

game/messages.py line 112 at r2 (raw file):

def subtitle_level_default():

If subtitle can be null/blank then I don't think we should need this.

---

game/messages.py line 116 at r2 (raw file):

def hint_level_default():

This is the default hint text, if you specify it in models you should be able to remove it from here. Be mindful of where this current function is being used though.

---

game/models.py line 159 at r2 (raw file):

        max_length=20, choices=character_choices(), blank=True, null=True, default=None
    )
    subtitle = models.TextField(max_length=10000, null=True)

You can reduce the length of this, for a subtitle I reckon you can make it 100 chars max.

---

game/models.py line 159 at r2 (raw file):

        max_length=20, choices=character_choices(), blank=True, null=True, default=None
    )
    subtitle = models.TextField(max_length=10000, null=True)

Add blank=True

---

game/models.py line 160 at r2 (raw file):

    )
    subtitle = models.TextField(max_length=10000, null=True)
    lesson = models.TextField(max_length=10000, null=True)

It seems like we'll have a default lesson if the user doesn't enter one. As such, please remove null=True, and add a default equal to our default lesson text.

---

game/models.py line 161 at r2 (raw file):

    subtitle = models.TextField(max_length=10000, null=True)
    lesson = models.TextField(max_length=10000, null=True)
    hint = models.TextField(max_length=10000, null=True)

It seems like we'll have a default hint if the user doesn't enter one. As such, please remove null=True, and add a default equal to our default hint text.

---

game/static/game/js/animation.js line 353 at r2 (raw file):

			var otherMsg = "";
			if (animation.popupHint) {
				console.log("popuphint");

Remove

---

game/static/game/js/animation.js line 357 at r2 (raw file):

				otherMsg = '<div id="hintBtnPara">' + '</div><div id="hintText">' + HINT + '</div>';
			}
			console.log(buttons);

Remove

---

game/static/game/js/level_editor.js line 194 at r2 (raw file):

        tabs.blocks = new ocargo.Tab($('#blocks_radio'), $('#blocks_radio + label'), $('#blocks_pane'));
        tabs.random = new ocargo.Tab($('#random_radio'), $('#random_radio + label'), $('#random_pane'));
        tabs.instruction = new ocargo.Tab($('#instruction_radio'), $('#instruction_radio + label'), $('#instruction_pane'));

After checking with Laura, we should rename this to tabs.description

---

game/templates/game/level_editor.html line 508 at r2 (raw file):

                <textarea id="instruction" rows="4" cols="50"></textarea>
              </div>
            </div>

There seems to be an issue with this section not showing up (same problem in hint pane).

Pausing my review so you can have a look.

Code quote:

            <div class="tab_pane_content_holder">
              <div class="tab_pane_content">
                <p>{% trans "<b>Subtitle</b><br>What is the subtitle of this level?" %}</p>
                <textarea id="subtitle" rows="4" cols="50"></textarea>

                <p>{% trans "<b>Instruction</b><br>What do players have to do to complete this level?" %}</p>
                <textarea id="instruction" rows="4" cols="50"></textarea>
              </div>
            </div>

[faucomte97]

Reviewed 1 of 15 files at r1, 4 of 13 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @evemartin)

---

game/end_to_end_tests/test_level_editor.py line 5 at r3 (raw file):

from selenium.webdriver.support import expected_conditions as EC
from selenium.webdriver.support.ui import Select
from selenium.webdriver.support.ui import WebDriverWait

Combine these imports pls

Code quote:

from selenium.webdriver.support.ui import Select
from selenium.webdriver.support.ui import WebDriverWait

---

game/end_to_end_tests/test_level_editor.py line 199 at r3 (raw file):

            EC.visibility_of_element_located((By.ID, "myModal-mainText"))
        )
        hint_modal_text_three = self.selenium.find_element(By.ID, "myModal-mainText").get_attribute("innerHTML")

Why three? 😅

---

game/end_to_end_tests/test_level_editor.py line 201 at r3 (raw file):

        hint_modal_text_three = self.selenium.find_element(By.ID, "myModal-mainText").get_attribute("innerHTML")
        assert "test hint" in hint_modal_text_three
        

Blank spaces - run Black

---

game/static/game/image/icons/instruction.svg line 0 at r3 (raw file):
Rename to description.svg

---

game/static/game/js/game.js line 10 at r3 (raw file):

  this.currentlySelectedTab = null
  this.isMuted = Cookies.get('muted') === 'true'
  this.hasTimedHintAppeared = false

No need for this as we're no longer doing timed hints for now

---

game/static/game/js/game.js line 289 at r3 (raw file):

ocargo.Game.prototype.registerFailure = function () {
  this.failures += 1
  return this.failures >= 1

Any idea why this was a 3 before, and how come you changed it to 1?

---

game/static/game/js/level_editor.js line 194 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

After checking with Laura, we should rename this to tabs.description

(along with every other instance, sorry 😬)

---

game/templates/game/level_editor.html line 202 at r3 (raw file):

          <label for="instruction_radio">
            <img src='{% static "game/image/icons/instruction.svg" %}'>
            <span>{% trans "Instruction" %}</span>

Description

---

game/templates/game/level_editor.html line 511 at r3 (raw file):

                <p>{% trans "<b>Instruction</b><br>What do players have to do to complete this level?" %}</p>
                <textarea id="instruction" rows="4" cols="50"></textarea>

There's no save button after this (check with Laura)

[evemartin]

Reviewable status: 3 of 19 files reviewed, 19 unresolved discussions (waiting on @faucomte97)

---

game/messages.py line 104 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

This isn't used anywhere - please remove (I know you didn't work on this but it ties in closely with what this PR is doing)

Done!

---

game/messages.py line 108 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

This is the default description/lesson text, if you specify it in models you should be able to remove it from here. Be mindful of where this current function is being used though.

Removed!

---

game/messages.py line 112 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

If subtitle can be null/blank then I don't think we should need this.

Removed!

---

game/messages.py line 116 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

This is the default hint text, if you specify it in models you should be able to remove it from here. Be mindful of where this current function is being used though.

Removed!

---

game/models.py line 159 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

You can reduce the length of this, for a subtitle I reckon you can make it 100 chars max.

Done!

---

game/models.py line 159 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Add blank=True

Done!

---

game/models.py line 160 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

It seems like we'll have a default lesson if the user doesn't enter one. As such, please remove null=True, and add a default equal to our default lesson text.

Updated!

---

game/models.py line 161 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

It seems like we'll have a default hint if the user doesn't enter one. As such, please remove null=True, and add a default equal to our default hint text.

Updated!

---

game/end_to_end_tests/test_level_editor.py line 5 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Combine these imports pls

Done!

---

game/end_to_end_tests/test_level_editor.py line 199 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Why three? 😅

I used to have another check to do with the hint modal text but when we got rid of the timer/trigger we didn't need that one anymore, I just forgot to rename 😅 Fixed!

---

game/end_to_end_tests/test_level_editor.py line 201 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Blank spaces - run Black

Formatted!

---

game/static/game/js/animation.js line 353 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove

Removed!

---

game/static/game/js/animation.js line 357 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove

Removed!

---

game/static/game/js/game.js line 10 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

No need for this as we're no longer doing timed hints for now

Removed!

---

game/static/game/js/game.js line 289 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Any idea why this was a 3 before, and how come you changed it to 1?

I think it was a 3 before so that the hint prompt only appeared on the failure modal after the player had made a few attempts - Stefan, Laura, and I discussed hint behavior on Friday and the conclusion we came to was that the prompt should appear on every failure modal since players can technically access the hint at any time via the level toolbar but that we would check in with teachers about hint prompting to see if this works for them!

---

game/static/game/js/level_editor.js line 194 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

(along with every other instance, sorry 😬)

Ahh that's okay, I have changed it everywhere I could find it!

---

game/templates/game/level_editor.html line 202 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Description

Changed!

---

game/templates/game/level_editor.html line 511 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

There's no save button after this (check with Laura)

After checking with Laura, we agreed to not include the button here for now and instead to wait until the autosave functionality is added!

---

game/static/game/image/icons/instruction.svg line at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Rename to description.svg

Renamed!

[faucomte97]

Reviewed 16 of 16 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @evemartin)

---

game/models.py line 159 at r2 (raw file):

Previously, evemartin wrote…

Done!

Ah, but we can leave null=True, right?

---

game/serializers.py line 86 at r4 (raw file):

            return description
        else:
            return "Can you find the shortest route?"

I have a feeling that we no longer need this in this way. Essentially, this piece of code checks if the level is a "default" level (ie, not custom) and if so, it gets the description from the large file with all the descriptions & titles. If not, it used to return the default description from that same file.

Now that we've moved the default description string to the model, we can probably do something like this:

return getattr(messages, "description_level" + obj.name)() if obj.default else obj.description

Let me know if that makes sense. If so, try it out and see if it works.

This is still not an ideal solution, ideally all the descriptions, hints, titles and subtitles would now be saved directly on the model level and not loaded from a file. But for now, this will do.

---

game/serializers.py line 93 at r4 (raw file):

            return hint
        else:
            return "Think back to earlier levels. What did you learn?"

Same logic as above.

---

game/static/game/js/game.js line 289 at r3 (raw file):

Previously, evemartin wrote…

I think it was a 3 before so that the hint prompt only appeared on the failure modal after the player had made a few attempts - Stefan, Laura, and I discussed hint behavior on Friday and the conclusion we came to was that the prompt should appear on every failure modal since players can technically access the hint at any time via the level toolbar but that we would check in with teachers about hint prompting to see if this works for them!

Oh I see, that's cool!

---

game/static/game/js/level_editor/owned_levels.js line 45 at r4 (raw file):

        delete level.name;

        console.log(level);

Remove :)

---

game/templates/game/level_editor.html line 503 at r4 (raw file):

          <div id="description_pane" class="tab_pane">
            <h2 class="title"><img class="modal_image" src='{% static "game/image/icons/description.svg" %}'>{% trans "Description" %}</h2>
            <p> {% trans "Give this level a subtitle and a description describing what to do within this level for your students." %} </p>

wording is a bit redundant - I think "Give this level a subtitle and a description of what to do within this level for your students" is OK

---

game/migrations/0094_add_description_hint__subtitle_to_levels.py line 26 at r4 (raw file):

            model_name='level',
            name='subtitle',
            field=models.TextField(default='', max_length=10000),

The max_length here is still 10000, I think you may need to re-make the migration.

[evemartin]

Reviewable status: 15 of 19 files reviewed, 6 unresolved discussions (waiting on @faucomte97)

---

game/models.py line 159 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Ah, but we can leave null=True, right?

If I omit null=True then an empty subtitle is saved and later printed as an empty string, whereas if I include it then it's saved as the value NULL and printed as the value None - I could provide the empty string as a default value but I think it will be cleaner to just leave out null=True!

---

game/serializers.py line 86 at r4 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

I have a feeling that we no longer need this in this way. Essentially, this piece of code checks if the level is a "default" level (ie, not custom) and if so, it gets the description from the large file with all the descriptions & titles. If not, it used to return the default description from that same file.

Now that we've moved the default description string to the model, we can probably do something like this:

return getattr(messages, "description_level" + obj.name)() if obj.default else obj.description

Let me know if that makes sense. If so, try it out and see if it works.

This is still not an ideal solution, ideally all the descriptions, hints, titles and subtitles would now be saved directly on the model level and not loaded from a file. But for now, this will do.

That does make sense! I have tested it out and it seems to work :)

---

game/serializers.py line 93 at r4 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Same logic as above.

Same fix as above!

---

game/templates/game/level_editor.html line 503 at r4 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

wording is a bit redundant - I think "Give this level a subtitle and a description of what to do within this level for your students" is OK

Done!

---

game/static/game/js/level_editor/owned_levels.js line 45 at r4 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove :)

Done!

---

game/migrations/0094_add_description_hint__subtitle_to_levels.py line 26 at r4 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

The max_length here is still 10000, I think you may need to re-make the migration.

Should be fixed now!

[faucomte97]

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @evemartin)

---

game/models.py line 159 at r2 (raw file):

Previously, evemartin wrote…

If I omit null=True then an empty subtitle is saved and later printed as an empty string, whereas if I include it then it's saved as the value NULL and printed as the value None - I could provide the empty string as a default value but I think it will be cleaner to just leave out null=True!

Where does it get printed as None? If the value of subtitle is None then it shouldn't get used / printed anywhere - if we ensure that's the case then maybe we can have null=True.
But if that's not really doable then I agree with what you're saying 🙂

[evemartin]

Reviewable status: 14 of 19 files reviewed, 1 unresolved discussion (waiting on @faucomte97)

---

game/models.py line 159 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Where does it get printed as None? If the value of subtitle is None then it shouldn't get used / printed anywhere - if we ensure that's the case then maybe we can have null=True.
But if that's not really doable then I agree with what you're saying 🙂

That's a very fair point - it was getting printed as None on the custom level starting popup, but it should be fixed now!

[faucomte97]

Reviewed 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @evemartin)

---

game/static/game/js/drawing.js line 1045 at r6 (raw file):

  buttons
) {
  console.log(subtitle)

Remove 😛

---

game/static/game/js/level_editor.js line 2632 at r6 (raw file):

                if(blocks[i].number) {
                    $('#' + type + '_number').val(blocks[i].number);
                }restoreState

is this a mistake copy paste? 🤔

[evemartin]

Reviewable status: 17 of 19 files reviewed, 2 unresolved discussions (waiting on @faucomte97)

---

game/static/game/js/drawing.js line 1045 at r6 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove 😛

I'm so sorry for this, removed 😓

---

game/static/game/js/level_editor.js line 2632 at r6 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

is this a mistake copy paste? 🤔

Yes it is, also removed 😅

[faucomte97]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @evemartin)

[evemartin]

Update before I go on holiday: on production, I have made a new ticket (linked on the other task) to address the emoji bug