cms icon indicating copy to clipboard operation
cms copied to clipboard

New analysis spoiler attachments

Open magula opened this issue 7 years ago • 4 comments

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

magula avatar Sep 15 '18 15:09 magula

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...)

wil93 avatar Sep 17 '18 20:09 wil93

@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.)

lw avatar Sep 17 '18 20:09 lw

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!

stefano-maggiolo avatar Sep 19 '18 09:09 stefano-maggiolo

@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.

magula avatar Sep 21 '18 21:09 magula