Features: Images in questions, Configurable amount of questions - #20
Features: Images in questions, Configurable amount of questions#20redshiftss wants to merge 4 commits into
Conversation
f9e4929 to
bf83da1
Compare
mxsasha
left a comment
mxsasha
left a comment
There was a problem hiding this comment.
Sorry for taking forever on this and thanks for making the PR! Good changes, but we can do some improvements :)
There was a problem hiding this comment.
This has a lot of duplication with the base Dockerfile, and I'm concerned they'll run out of sync. Maybe it can use the other one as a base?
There was a problem hiding this comment.
We should squash these migrations.
| def is_boolean(self): | ||
| answers = self.answer_set.filter(status=Answer.STATUS.active) | ||
| return "True" in list(self.answer_set.filter(status=Answer.STATUS.active)) and "False" in list(self.answer_set.filter(status=Answer.STATUS.active)) |
There was a problem hiding this comment.
This does not actually work. These are Answer instances, therefore never true/false.
| def is_boolean(self): | |
| answers = self.answer_set.filter(status=Answer.STATUS.active) | |
| return "True" in list(self.answer_set.filter(status=Answer.STATUS.active)) and "False" in list(self.answer_set.filter(status=Answer.STATUS.active)) | |
| def is_boolean(self): | |
| active_answer_texts = set( | |
| self.answer_set.filter(status=Answer.STATUS.active) | |
| .values_list("answer_text", flat=True) | |
| ) | |
| return "True" in active_answer_texts and "False" in active_answer_texts |
| questions_amt = self.object.questions_amount | ||
| else: | ||
| questions_amt = DEFAULT_TEST_GROUP_QUESTION_AMOUNT | ||
| random_pk = random.sample(list(pks), questions_amt) |
There was a problem hiding this comment.
This raises ValueError if the requested questions is more than the total amount we have. We need to validate for that.
| class Question(TimeStampedModel): | ||
| STATUS = Choices("active", "inactive") | ||
| question_text = models.TextField(unique=True) | ||
| question_image = models.ImageField(blank=True, null=True, upload_to="./uploads") |
There was a problem hiding this comment.
I'll need to sort out uploads in my deployment environment.
| postgresql18 | ||
|
|
||
| # set default password for postgres user | ||
| RUN echo "password" > passwd postgres |
There was a problem hiding this comment.
What does this mean? It just creates a file.
| return random.choice(remaining_questions) | ||
|
|
||
| def questions_answered(self): | ||
| return len(self.answers.all()) |
There was a problem hiding this comment.
More efficient (as you already did in total_questions):
| return len(self.answers.all()) | |
| return self.answers.count() |
| null=True, | ||
| blank=True, | ||
| ) | ||
| questions_amount = models.BigIntegerField(verbose_name="How many questions would you like the test to have? (optional, will default to 40 if left blank)", null=True, blank=True) |
There was a problem hiding this comment.
Also, how about default=40, null=False? Backwards compatible, and fits.
Also, let's use DEFAULT_TEST_GROUP_QUESTION_AMOUNT, which we can then move to settings/__init__.py, fits better there anyways.
| if self.object.questions_amount is not None: | ||
| questions_amt = self.object.questions_amount | ||
| else: | ||
| questions_amt = DEFAULT_TEST_GROUP_QUESTION_AMOUNT |
There was a problem hiding this comment.
Instead of this, I suggest a new method:
def clean_questions_amount(self):
return self.cleaned_data["questions_amount"] or DEFAULT_TEST_GROUP_QUESTION_AMOUNT
| It helps if you can tell us what exactly is wrong with the question, and reference specific sections of the WFTDA Rules/cases. | ||
|
|
||
| # Notes on style: | ||
| The only formatter we _particularly_ enforce is the use of `djLint` for formatting the HTML templates. |
There was a problem hiding this comment.
We should probably add this to CI, but that's out of scope here.
|
@mxsasha Sorry for taking so long to respond! I think I've addressed most of your comments |
2 participants
This PR addresses:
It also adds a (pretty barebones, unpopulated) dev environment, and some info on how to set that up locally in case anyone's interested in contributing. I have not yet added instructions on who to contact in case of wrong questions since I wasn't sure who would want their e-mail out in the open from the question-writing team.
@mxsasha should also note that there should be some kind of media volume that gets mounted to the docker container when deploying, similar to how we do it in the dev setup.