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

New analysis spoiler attachments #1027

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

magula
Copy link
Contributor

@magula magula commented Sep 15, 2018

Introduce analysis mode spoilers (task attachments visible to contestants only in analysis mode).
This can be used for providing additional (or all) test data, sample solutions or anything
that contestants are not allowed to access during the contest, but only during analysis.


This change is Reviewable

Introduce analysis mode spoilers (task attachments visible to contestants only in analysis mode).
This can be used for providing additional (or all) test data, sample solutions or anything
that contestants are not allowed to access during the contest, but only during analysis.
@wil93
Copy link
Member

wil93 commented Sep 17, 2018

Hi! Interesting idea. I'm not sure if it's a feature that everyone would need, but maybe it can save some time to contest admins.

At a first glance, however, there seem to be a lot of code duplication: do you need the Spoiler class at all? It might be easier to just have a new boolean flag in the existing Attachment class, to specify if the attachment is meant for the "contest mode" or "analysis mode". Better yet, an integer phase value (because CMS already has the concept of "contest phase", like phase = 0 should mean "contest not started yet", 1 should be "contest running" and so on...)

@lw
Copy link
Member

lw commented Sep 17, 2018

@stefano-maggiolo and I were just discussing this. This could be implemented in several ways (distinct tables, boolean column, polymorphism), each with pros and cons. Stefano seems to prefer the current approach, I prefer polymorphism. My concern with a boolean column is that one needs to remember to always filter by it and, if one forgets, one could end up displaying "spoilers" during the contest. (Also, we're not fully happy with the name "spoiler", though we couldn't come up with anything better.)

@stefano-maggiolo
Copy link
Member

Yeah, I prefer it as it is, and possibly with the boring name "AnalysisAttachment".

I've been sick, and now I'm off for a week or so, apologies for any (further) delay. Thanks @magula for the patch, I like the concept!

@magula
Copy link
Contributor Author

magula commented Sep 21, 2018

@lerks in fact described pretty well the reason I decided not to use a boolean flag.

Using a boolean flag instead of just duplicating the code wouldn't even eliminate all of the duplicate code, but I think it would result in code in which it's easier to make an error, and one would have to be more careful whenever one's changing anything about attachments or spoilers not to accidentally... spoil spoilers.

Now, I don't have any strong preferences on whether to use polymorphism or stick to the current approach (I'm not sure about the benefits of using polymorphism here or how to do that best).

About the name: We weren't too happy about it either, but it's the best we could come up with. :) "Analysis attachments" would certainly be a good option, but I was a bit afraid that it might be to easy to confuse with regular attachments at a first glance (that's just guessing, though). So I'm definitely fine with changing that name.

@stefano-maggiolo stefano-maggiolo self-requested a review September 26, 2018 15:56
Copy link
Member

@stefano-maggiolo stefano-maggiolo left a comment

Choose a reason for hiding this comment

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

To make a decision:

  • let's keep it simple and stick to separate tables, also no polymorphism solution to code duplication;
  • rename to AnalysisAttachment(s).

You also need to update the DB schema (in db/__init__.py) and provide an updater that adds analysis_attachment = {} to every task. See 5e48afe for an example.

Reviewed 13 of 13 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @magula)


cms/db/init.py, line 109 at r1 (raw file):

from .user import User, Team, Participation, Message, Question
from .task import Task, Statement, Attachment, Spoiler, Dataset, Manager, \
    Testcase

Also add to __all__


cms/db/task.py, line 359 at r1 (raw file):

class Spoiler(Base):
    """Class to store additional files to give to the user together
    with the statement of the task during analysis mode.

Class to store additional attachments to show only during analysis mode.


cms/server/contest/templates/task_description.html, line 202 at r1 (raw file):

{% if contest.analysis_enabled and actual_phase == +3 and task.spoilers|length > 0 %}
    <h2>{% trans %}Analysis Spoilers{% endtrans %}</h2>

From the point of view of the contestants I think it's a bit silly to make a name distinction between regular attachments and spoilers/analysis attachments. How about just having a single list, with analysis attachments at the beginning and in bold, for example?

@wil93 wil93 changed the base branch from master to main December 26, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants