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: Deliver at non-destination does not show popup #1694

Merged
merged 5 commits into from
Aug 7, 2024

Conversation

Ridwanah
Copy link
Contributor

@Ridwanah Ridwanah commented Aug 5, 2024

Description

I added a popup for when a delivery is made at a location which is not a valid destination. This popup informs the user that
they tried to deliver at a destination that doesn’t exist. The failure sound now plays when this popup appears and this event is logged as a reason for termination when it occurs.

How Has This Been Tested?

I tested this using pytest. It passed all 58 tests. The selenium tests did not run.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have linked this PR to a ZenHub Issue.

This change is Reviewable

Sorry, something went wrong.

@Ridwanah
Copy link
Contributor Author

Ridwanah commented Aug 5, 2024

Hi, this is including all of my old commits for another issue. I tried to merge commits with origin master but it does nothing in my terminal. Git status says that it is up to date but on github, it says that I am 8 commits behind.

@faucomte97
Copy link
Contributor

Hi @Ridwanah, I reckon what happened is that you kept working on the same branch, or maybe you didn't pull the changes from your latest work into your local repo before starting this task.
You may need to pull from the upstream repo (aka our repo, not your fork) first.

@Ridwanah
Copy link
Contributor Author

Ridwanah commented Aug 6, 2024

Hi @faucomte97, thanks for your answer. I made a branch, so I don't lose my work, and pulled the changes from upstream. I did this in rapid router, but I am having a problem now that when I run the code, it gives me an error, related to aimmo, which makes me think that I pulled the changes only for rapid router and it is affecting other submodules. I am not sure, here is the error: `Traceback (most recent call last):
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/template/utils.py", line 66, in getitem
return self._engines[alias]
KeyError: 'django'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "./example_project/manage.py", line 10, in
execute_from_command_line(sys.argv)
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/core/management/init.py", line 419, in execute_from_command_line
utility.execute()
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/core/management/init.py", line 413, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/core/management/base.py", line 354, in run_from_argv
self.execute(*args, **cmd_options)
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/core/management/base.py", line 398, in execute
output = self.handle(*args, **options)
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/core/management/base.py", line 89, in wrapped
res = handle_func(*args, **kwargs)
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/core/management/commands/migrate.py", line 75, in handle
self.check(databases=[database])
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/core/management/base.py", line 419, in check
all_issues = checks.run_checks(
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/core/checks/registry.py", line 76, in run_checks
new_errors = check(app_configs=app_configs, databases=databases)
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/contrib/admin/checks.py", line 78, in check_dependencies
for engine in engines.all():
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/template/utils.py", line 90, in all
return [self[alias] for alias in self]
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/template/utils.py", line 90, in
return [self[alias] for alias in self]
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/template/utils.py", line 81, in getitem
engine = engine_cls(params)
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/template/backends/django.py", line 25, in init
options['libraries'] = self.get_templatetag_libraries(libraries)
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/template/backends/django.py", line 43, in get_templatetag_libraries
libraries = get_installed_libraries()
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/template/backends/django.py", line 108, in get_installed_libraries
for name in get_package_libraries(pkg):
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/template/backends/django.py", line 121, in get_package_libraries
module = import_module(entry[1])
File "/usr/local/python/current/lib/python3.8/importlib/init.py", line 127, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
File "", line 1014, in _gcd_import
File "", line 991, in _find_and_load
File "", line 975, in _find_and_load_unlocked
File "", line 671, in _load_unlocked
File "", line 843, in exec_module
File "", line 219, in _call_with_frames_removed
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/portal/templatetags/app_tags.py", line 1, in
from aimmo.templatetags.players_utils import get_user_playable_games
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/aimmo/templatetags/players_utils.py", line 1, in
from aimmo.models import Game
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/aimmo/models.py", line 50, in
class Game(models.Model):
File "/codeforlife-workspace/rapid-router/.venv/lib/python3.8/site-packages/django/db/models/base.py", line 113, in new
raise RuntimeError(
RuntimeError: Model class aimmo.models.Game doesn't declare an explicit app_label and isn't in an application in INSTALLED_APPS.`

@faucomte97
Copy link
Contributor

Hi @Ridwanah, ah yes, we made a significant change last week which would be causing this.
Please delete your Pipfile.lock file in the root folder and re-run pipenv install --dev. It should fix your issue - let me know if it doesn't!

@Ridwanah
Copy link
Contributor Author

Ridwanah commented Aug 6, 2024

Hi @faucomte97, it works now. Thanks.

@Ridwanah
Copy link
Contributor Author

Ridwanah commented Aug 6, 2024

When I run the python tests, I get an error related which say 'no fixture named aimmo_characters found'. I know I haven't merged this branch properly yet, I will do so. The branch i am working on that i called backup-branch is exactly the same with only one commit ahead, which is updating pipfile.lock, has the same error when I run the python tests.

@faucomte97
Copy link
Contributor

faucomte97 commented Aug 6, 2024

Hmm, I'm not 100% sure why this is happening. I'm looking into this now and will let you know if I find anything.
For context, we've remove aimmo last week and most of its related code. The error you're getting is pointing to one of the tests in the cfl_common package which is in the codeforlife-portal repo, but since you've managed to install the latest version of portal, the tests shouldn't be an issue anymore...

@faucomte97
Copy link
Contributor

Hey @Ridwanah, just a quick update to let you know that I'm actually getting this error on my side too so it seems we still need to clean up a few things. I'll let you know as soon as we sort it out. Apologies for the blocker

@faucomte97
Copy link
Contributor

Hi @Ridwanah, we've managed to fix the issue which involved upgrading the version of our common package. Please delete Pipfile.lock and re-run pipenv install --dev one more time and the tests should pass!

@Ridwanah
Copy link
Contributor Author

Ridwanah commented Aug 7, 2024

Hi, the tests work now.

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 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Ridwanah)


game/static/game/js/model.js line 390 at r2 (raw file):

        this.makeDelivery(destination, 'DELIVER');
    } else {
        ocargo.animation.appendAnimation({

Please add the following line before this line and after the else:

ocargo.game.sendAttempt(0);

This is just so the level marks this an attempt with 0 points.

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 and just left 1 comment 🙂

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Ridwanah)

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 r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Ridwanah)

@faucomte97 faucomte97 merged commit 9e1f4b0 into ocadotechnology:master Aug 7, 2024
4 of 5 checks passed
@faucomte97
Copy link
Contributor

Well done @Ridwanah! 2nd PR merged in! 🥳
We will proceed with our manual tests as soon as it gets deployed to our staging environment.

@Ridwanah
Copy link
Contributor Author

Ridwanah commented Aug 7, 2024

Thank you for your support :). I am looking forward to doing more contributions.

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

2 participants