[matheuszych]

See: https://mantis.ilias.de/view.php?id=45782

The test participants listing always summarized attempt overview data for scored attempts only. ParticipantTable now renders a UI mode control (scored attempts vs last attempts), keeps the choice in the view_mode request parameter via RequestDataCollector::getViewMode(), and passes ilCtrl into the table for proper link targets. Results\Data\Factory::addAttemptOverviewInformationToParticipants accepts a flag to target the last started pass when building overviews. English and German assessment language entries cover the new labels.

/cc @thojou

[dsstrassner]

@kergomard please review!

[jweber-unibe]

Possibly I am just mistaken – but I fail to see the direct connection of this PR to the mentioned issue, implied by the Mantis link. 🤔
The issue report in itself has nothing to do with manual scoring but deals with the status display in the 'Participants' table.

(... just in case this remark has any relevance at all 😉)

[kergomard]

Thank you very much for the PR @matheuszych

First: I would echo the comment from @jweber-unibe : Could you tell me what problem you are trying to solve and where the corresponding behavior is specified? A link to an issue where @dsstrassner agreed that this is the right way to go, would completely suffice.

Then:

• This looks potentially unnecessarily expensive we somehow retrieve a whole dataset to get one single small information and simply rely on the fact that it will be cached. If we think we need this information: Shouldn't it be available in a dataset we will really need at these points?
• This will look completely different for ILIAS 11, though, right? Could you please provide a PR for ILIAS 11. If this PR can be made more elegant, we can stick to the solution here for ILIAS 10 as we would have an immediate exit ramp.

Thank you very much again and best,
@kergomard

[dsstrassner]

@kergomard & @matheuszych I think @matheuszych should report about this in the TechSquad.

[jweber-unibe]

Relating to my comment from above (Apr 30) ...

Possibly I am just mistaken – but I fail to see the direct connection of this PR to the mentioned issue, implied by the Mantis link. 🤔 The issue report in itself has nothing to do with manual scoring but deals with the status display in the 'Participants' table.

→ Possibly this PR was rather meant to point to https://mantis.ilias.de/view.php?id=44871?!
The PR's description fits much better there ...

(Maybe(?!) also of interest in this context: https://mantis.ilias.de/view.php?id=48278 / #12028)

[matheuszych]

Hello @jweber-unibe and @kergomard,

First of all, thank you for your patience! I believe I now know the reason behind these changes.

Before going into that, I would like to point out that this is not related to Mantis issue 44871, as the timelines do not match. This Mantis issue had already been closed long before I attempted to fix this issue.

While going through our internal notes, I came across a comment of my own that provides much of the explanation:

... This fix is technically not really for that Mantis issue either. ...

However, this does not mean that the PR should be abandoned, as it consists of two separate commits:

• Test: Prevent manual scoring of unfinished attempts
• Test: Add participant table attempt view modes

The second commit is directly related to Stephan’s comment in Mantis issue 45782 and is therefore still relevant in my opinion.

Although the first commit is not directly related to this issue, I would still like it to be considered as a possible change to ILIAS. In my opinion, grading a test pass should not be possible before the test taker has fully completed it, as they can still change their answers. Consequently, the grading may not be accurate. Please correct me if I am mistaken. Otherwise, I would be happy to open a separate Mantis report and move this commit to a separate PR.

I should have provided this explanation from the beginning to avoid any confusion. I apologize for that.

My next step will probably be to extract the first commit from this PR.

Best regards,
@mzych

[dsstrassner]

@kergomard will you look into this PR before your absence, or should @thojou take over?

I also think that only a finished test attempt should be scored by a person in exams. But what is for long-lasting self-assessments and situations where only automatic scoring comes into use?
Where the instant feedback is activated. Or settings where preconditions with result ranges are used? Is this still possible, although no one has clicked on the 'Finish Test' button or the time has run out or our wonderful test has else in his box of much loved options available?

[kergomard]

Thank you for the explanations @matheuszych !

I think we get some order in the house here before looking deeper into this:

• First, I would suggest to take the comment of @dsstrassner very seriously, especially the limitation. Even if we probably do agree, that only finished attempts should be gradable in exams, the case for quizzes is a completely different one. I do not think we can do that.
• Please then provide a PR for the rest of the changes in here and probably a second one porting it to ILIAS 11 as it will look different there.

I hope this is understandable otherwise we might find a minute to discuss the way ahead in the next weeks (with a little bit of planing ;-)).

Thanks and best,
@kergomard

[matheuszych]

Hello @kergomard , Hello @dsstrassner ,
thank you for your explanations!

I have dropped one of the commits for now to just keep the attempt view modes.

Best regards
@matheuszych

[matheuszych]

PR for ILIAS 11 #12229

[kergomard]

Thank you very much for the improvement @matheuszych !

I've a few small things:

• Please make all the constants in ParticiapntTable private.
• Could you please keep the order of $scored_attempts_label and $last_attempts_label consistent on lines 124-127 of the ParticipantTable. The reversed order comes as a surprise.
• Please treat variables as immutable as far as possible and do not assign and return in one go (this is very error prone). In this case here, we have a problem with ParticipantTable::loadRecords(): I think it is definitely misnamed. We should split it up into a ParticipantTable::loadRecords() that does not return anything and is only called, if necessary, and a function ParticipantTable::retrieveRecords(): iterable` that actually returns the records.
• I do not think the parameter $last_attempt as a bool in Factory::addAttemptOverviewInformationToParticipant() is a good idea as it does not convey enough information (what attempt do we add if it is not the last)? Please introduce a little Enum AttemptSpecificers that currently has two cases ScoredAttempt, LastAttempt. I think it is best to back it by the language variable and to move all building of the ViewControl to it. We might in the future have more AttemptSpecifiers, e.g. Deleted.

I think @dsstrassner will need to check this for usability, as I'm really unsure, if we are not missing anything here. This table is rather versatile and it has already been refactored, thus it is especially important to not worsen the situation.

Thank you very much again and best,
@kergomard

[matheuszych]

Hello @kergomard ,
thank you again for you feedback. I applied your suggestions and added a slight improvement.

Please take a quick look. If everything is correct. If so, i will squash the commits.

Best regards
@matheuszych

[kergomard]

Thank you very much @matheuszych !

I approve this like this, if you find the time, I would not change the things you changed, but I do not care enough (I will simply remove this again, once I see it):

• I would prefer if there weren't two assignments to $records in ParticipantTable::loadRecords(). It can easily be avoided.
• The line 157 of the Factory ($participant_with_start = $participant) gives the illusion that $participant_with_start is somehow in any way different from $participant, but it isn't. It's exactly the same object, so after the next assignment both will have changed. I would prefer it not being there. In my view this is a reasonable case for not treating a variable as immutable: You have received a variable as a parameter and you need to update it under certain conditions (never if you need to updated it unconditionally).
• I will always give the primary variable in an anonymous function the name $v unless, and here it can be argued this is the case, the function is too long.

Please make sure the changes are in both: This version and the one for ILIAS 10 and then @thojou : Please merge, this way I would now concentrate on the Questions-PR ;-).

...and @dsstrassner would, as stated above, maybe like to test this change.

Thanks again and best,
@kergomard

[matheuszych]

Hello @kergomard ,
thank you again for your feedback. I adjusted the code to your suggestions.

Best regards
@matheuszych

[thojou]

Hey all,

thanks for your input.
So I'll wait with merging until @dsstrassner gives his final go.

Kind regards,
@thojou

[matheuszych]

ILIAS 12: #12330