-
-
Notifications
You must be signed in to change notification settings - Fork 775
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
Branch for working on gitcoin bounty https://gitcoin.co/issue/gitcoinco/web/5339/3562 #5979
Conversation
Added the new quest type to the quests creation form
Codecov Report
@@ Coverage Diff @@
## master #5979 +/- ##
==========================================
+ Coverage 27.56% 27.57% +<.01%
==========================================
Files 282 283 +1
Lines 26148 26245 +97
Branches 3850 3865 +15
==========================================
+ Hits 7208 7236 +28
- Misses 18675 18744 +69
Partials 265 265
Continue to review full report at Codecov.
|
…de battle style. - Modified the template file new.html - Modified the javascript in order to make it possible to chooe among quests types. If users choose the Code Battle quest type, the business logic to add and handle question is modified
…Working on quests editing now
…ugs too: 1. the tooltip were not added to new questions; 2. the remove question buttons did not work properly
Query to insert a test quest
|
hi @owocki ! Any news about this? Let me know if it can work or if it needs modifies, so I can schedule some time to work on it! Sorry to bother you! Thanks! |
sorry i've been on parenteal leave so some things dropped. mind attaching a quick video of the gameplay so that people can see what its like? do traditional quests still work on this branch? |
Hi Kevin! Yes, traditional quests still works. I'll make a video very soon and then I'll be back here. |
Hey Kevin. I have an issue with docker. I tried to fetch the upstream repo, and then I checkout my "battle-code-quest" repo and tryied to build the docker composer. However, now it doesn't build anymore, and it gives me me this error: Collecting pyshorteners
Downloading pyshorteners-1.0.0.tar.gz (9.8 kB)
ERROR: Command errored out with exit status 1:
command: /usr/bin/python3 -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-install-5o2gds2v/pyshorteners/setup.py'"'"'; __file__='"'"'/tmp/pip-install-5o2gds2v/pyshorteners/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' egg_info --egg-base /tmp/pip-install-5o2gds2v/pyshorteners/pip-egg-info
cwd: /tmp/pip-install-5o2gds2v/pyshorteners/
Complete output (7 lines):
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/tmp/pip-install-5o2gds2v/pyshorteners/setup.py", line 8, in <module>
README = r.read()
File "/usr/lib/python3.6/encodings/ascii.py", line 26, in decode
return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 21: ordinal not in range(128)
----------------------------------------
ERROR: Command errored out with exit status 1: python setup.py egg_info Check the logs for full command output.
ERROR: Service 'web' failed to build: The command '/bin/sh -c pip3 install --upgrade -r test.txt' returned a non-zero code: 1 Have you made changes in the docker-container recently? |
I think that it is related to the URL shortening, I opened an issue for that #6160 |
this is fixed in ebdb220 |
Hi Kevin! Here you have animated gif of the new quest Mechanic. Quest Creation (too big to put it here) |
pulling out the relevant frame => https://bits.owocki.com/eDuxyBWx this loops super cool. i guess the answer is computed and marked correct locally in the users javascript enviornment? we just need to be careful about XSS here |
Hi Kevin! So, everything is made in this two lines of code (in quests/quest_types/code_battle_style.py, lines 68 and 69) correct_answers = [ele['answer'] for ele in this_question['responses'] if ele['correct']] if this_question['question_type'] != 'boss_fight_question' else [re.sub(r"[\n\t\s]*", "", ele['answer']) for ele in this_question['responses'] if ele['correct']]
their_answers = [unescape(ele) for ele in payload.get('answers')] if this_question['question_type'] != 'boss_fight_question' else [re.sub(r"[\n\t\s]*", "", unescape(ele)) for ele in payload.get('answers')] and the check is made in line 75 did_they_do_correct = set(correct_answers) == set(their_answers) or (this_question.get('any_correct', False) and len(their_answers)) Now, as I said, this is very very naive and there is room for a lot of improvements. But, I was waiting for your feedback and suggestions on how to make it less naive, without creating security risks. I was thinking to evaluate the javascript (or solidity) code of users on server-side and to compare the results with the evaluation of code from quests' creator. But, I'm not super-skilled in python so, do you think that this is possible? |
thanks; yeah i think that architecting this in such a way thats not naive but also doesnt have a bunch of security holes / surface area for injection attacks will be a challenge. it def needs to work if variable names are diffrent for xample |
Thank you Kevin! So, what do you think is the best way to procede?
What do you think? |
Hi @owocki and @octavioamu I have finished the creation and edit pages and business logic for the new quests game mechanic. Checkout the animated GIF below. Anyway, I found two collateral bugs while creating this (though they didn't blocked me to code). Let me know if you want that I open two issues for them:
Anyway, let me know what you thing about this way to handle the new quest mechanics. I need a couple of hours to develop the part related to playing the quest, and another couple of hours to make some code refactoring, and then it should be done. Below the videos (sorry, I know that animated GIFs are boring because you are not able to fast-forward or fast-backward, but my laptop is to slow to record videos while docker is running). Have a nice week-end guys. Ciao!
|
Hi @owocki and @octavioamu I have developed the business logic for playing the new quest mechanic. Notes:
All the above things can be improved, but it would take time, we can talk on how to improve it. Have a nice evening! |
seems pretty promising :) thanks for the comprehensive write up! ill add this to the @gitcoinco/engineers PR review board, but maybe they'd prefer i do the review since i wrote quests? also did you regression test the old quests to make sure nothing has broken? |
Hi Kevin! Yes, I did some test on the old quests and nothing seems to be broken. Let me know if you find bugs or dangerous things in the code while reviewing it. I'll fix them quickly! |
hi @owocki , just a ping on this! |
sorry for the raido silence. @cervoneluca can you give me a dump of your quests table in your DB so i can ingest it and test? im having a hard time creating my own on your PR for some reason.. |
Hi @owocki, first thing first, I fixed a minor issue, so perhaps you want to pull the code again. I'm not sure if you need the whole dump or just the quest's one, so I'll attach both of them questshttps://www.dropbox.com/s/0zoisc7oz5nkla7/quests-16042020.dump?dl=0 whole gitcoinhttps://www.dropbox.com/s/y5l1caq1wuk4ldy/gitcoin-16042020.dump?dl=0 |
hi guys. Any chance this will be published soon or later? 😊 |
testing now |
for the quest/new page, i found the ux to e a bit confusing. the system should probalby proimpt the user to let them know what exactly they're supposed to do with both code inputs (bot the sample function and the test function) right? also should there be a title that promps them with what they're supposed to do? ( https://bits.owocki.com/p9u7ey9v ) whats the point of having two quiz types, a code battle + regular quiz, if were just going to allow the user to select on a per question basis the question type in the former? any chance you've got a couple test quetss that you8've created that you8 can do a psql dump for me on ? i cant seem to load the file you gave me, what am i doing wrong?
|
Hi Kevin!
First thing first, the idea to allow quests creator to write down a test function was intended to make the system more engaging for quests creators too.
You're right, it is possibile to add a "boss fight question" to quiz-style quests and so, it is possible to avoid a whole new quests type. However, the original idea here, was to create a new type of quest in a modular way, so the chances that it breaks the "quiz-style" quests are minimal. I think that when the new quest type is well-tested, then we can work on "merging" the quiz-style and the code-battle style.
In this comment there are the links to two dumps, one for the whole DB and one for the quests table. I don't know why you are receiving that error when trying to inject data on your DB instance (I'm not very-expert of django, indeed). However, I saw that in this file, there are these lines at the very start:
Maybe, you should be able to import the data after having deleted those lines. Let me know! |
@@ -136,36 +136,40 @@ var orb_state = function(state) { | |||
|
|||
$(document).ready(function() { | |||
|
|||
$('body').keyup(function(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most of these are indentation cahnges except the if statement?
</div> | ||
`; | ||
const boss_fight_answer_template = ` | ||
<hr> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mighte be worth moving these to JSX for basic sanity reasons :)
from ratelimit.decorators import ratelimit | ||
|
||
html_escape_table = { | ||
"&": "&", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm this file seems like its copied/pasted no? maybe worth DRYing it up
{% block 'content' %} | ||
<script type='text/javascript' src='//www.midijs.net/lib/midi.js'></script> | ||
<link rel="stylesheet" href="{% static 'v2/css/jquery-ui.css' %}" /> | ||
<link href="https://fonts.googleapis.com/css?family=Press+Start+2P&display=swap" rel="stylesheet"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also seems like it could be DRY'd up
@gitcoinco/engineers do u all want me to just handle code review on this one, since quests was my YOLO-baby? |
i wonder to what extent we should be worried about JS injection attackson this. from one angle, it would seem to me that JS injection is the whole point of this... but on the other hand, id hate if peoples metamask got hacked or something (and i can see an attacker maybe submitting a quest with nefarious JS in it, expecting us to test it / approv eit) |
@owocki, do I have to do something on my side ? (in the meanwhile that you and the engineers review the code) |
i left some inline comments on the code. some of the more egregious copy/paste should be cleaned up if this is gonna get into prod. also concerned about the security issues of JS injection. i also think the UX of new quest is confusing, so can work with u on how to make it more obvious |
closing this unless someone wants to pick it back up |
Description
Modified the model and the quests creation form in order to allow a new quest style.
The work is related to the following bounty on gitcoin.
https://gitcoin.co/issue/gitcoinco/web/5339/3562
Refers/Fixes
Refs: #5339
Testing
This commit is mostly to create the new branch so a test is not needed