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

fix: fix scoring for custom levels #1624

Merged
merged 31 commits into from
May 31, 2024

Conversation

evemartin
Copy link
Contributor

@evemartin evemartin commented May 23, 2024

This change is Reviewable

@evemartin
Copy link
Contributor Author

The edits to game.html are just for tabbing - I had originally made some other test-related changes to that file which I later got rid of but the formatting change is still there. I'm happy to get rid of it if you feel it makes the PR overly complicated but I have left it for now since it felt strange to untidy it on purpose!

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

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


game/tests/test_level_editor.py line 401 at r3 (raw file):

        self.login(email1, password1)
        level = create_save_level_with_multiple_houses(teacher1)

(Read this comment after you've read my other one)

If you disable the algo score on creation, then you could add a check right after this line to assert that the algo score is in fact disabled 🙂


game/views/level.py line 41 at r3 (raw file):

def play_custom_level(request, levelId, from_editor=False):
    level = cached_custom_level(levelId)
    level.disable_algorithm_score = True

While I understand that this specific line is the fix for the issue, I believe it should happen upon level creation / save to DB rather than when the level is loaded to be played - if that makes sense.
Essentially, what I'm saying is that the level's algorithm score should be disabled as soon as the level is created and saved to the DB.

Side question: how are you disabling the algorithm score without doing level.save()? 😱 🤔

Copy link
Contributor Author

@evemartin evemartin left a comment

Choose a reason for hiding this comment

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

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


game/tests/test_level_editor.py line 401 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

(Read this comment after you've read my other one)

If you disable the algo score on creation, then you could add a check right after this line to assert that the algo score is in fact disabled 🙂

Added :) I ended up using a mock since the data values don't end up getting returned as part of the response!


game/views/level.py line 41 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

While I understand that this specific line is the fix for the issue, I believe it should happen upon level creation / save to DB rather than when the level is loaded to be played - if that makes sense.
Essentially, what I'm saying is that the level's algorithm score should be disabled as soon as the level is created and saved to the DB.

Side question: how are you disabling the algorithm score without doing level.save()? 😱 🤔

I agree that it makes more sense to save the level with disabled_algorithm_score = True in the database and have made the change to reflect that in my most recent push, but we also have to address the custom levels that were made before the change, since they won't necessarily be saved again, just loaded - will we need a migration for this?

I think that with the way I had it before the algorithm score was getting disabled just before the level got loaded, so when it loaded it would have this property - it just wouldn't save to the database 😅

Copy link
Contributor Author

@evemartin evemartin left a comment

Choose a reason for hiding this comment

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

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


game/views/level.py line 41 at r3 (raw file):

Previously, evemartin wrote…

I agree that it makes more sense to save the level with disabled_algorithm_score = True in the database and have made the change to reflect that in my most recent push, but we also have to address the custom levels that were made before the change, since they won't necessarily be saved again, just loaded - will we need a migration for this?

I think that with the way I had it before the algorithm score was getting disabled just before the level got loaded, so when it loaded it would have this property - it just wouldn't save to the database 😅

** until there is a plan to update the old custom levels, I have left this line in just in case!

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

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


game/tests/test_level_editor.py line 412 at r4 (raw file):

        assert response.status_code == 200
        assert disable_algorithm_score == True

no need to have == True

Copy link
Contributor Author

@evemartin evemartin left a comment

Choose a reason for hiding this comment

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

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


game/tests/test_level_editor.py line 412 at r4 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

no need to have == True

Fixed :)

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

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

@evemartin evemartin merged commit 0249354 into master May 31, 2024
5 checks passed
@evemartin evemartin deleted the fix--fix-algorithm-score-in-created-levels branch May 31, 2024 09:39
evemartin added a commit that referenced this pull request May 31, 2024
* fix scoring for custom levels

* debug test

* debug test

* debug test

* debug test

* debug test

* debug test

* debug test

* debug test

* debug test

* debug test

* debug test

* tidy code

* tidy code pt 2

* Merge branch 'master' into fix--fix-algorithm-score-in-created-levels

* address PR comments

* debug test

* debug test

* try patching

* debug test

* address PR comments, fix test

* add back line

* add migration, remove redundant line

* address PR comments

* add property

* fix typo

* add log lines

* debug test

* debug test

* test change to ajax url

* revert changes used for testing
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.

2 participants