assignment_count_real_submissions() assumes all rows with a positive timestamp are "real submissions" when this is not true for upload assignment types

(a.assignmenttype <> 'upload' OR s.data2 = 'submitted') AND
(a.assignmenttype <> 'uploadsingle' OR s.numfiles > 0) AND

Are the ANDs and ORs in that the correct way around? At first glance it looks like it should be...

((a.assignmenttype <> 'upload' AND s.data2 = 'submitted') OR
(a.assignmenttype <> 'uploadsingle' AND s.numfiles > 0)) AND

From the testing instructions.

5. Apply fix

Don't put that in your testing instructions. Someone testing this will likely be pulling the latest from the integration repository then running a bunch of tests. They won't be just testing this so they will have your fix applied before they even start.

Regarding the SQL I believe I have it correct and my tests appear to confirm it. Basically I'm saying they both have to evaluate to true for a particular row to be returned.

So this says that if the assignment type is upload then it must also have data2 set to submitted:

(a.assignmenttype <> 'upload' OR s.data2 = 'submitted')

The second line says if the assignment type is uploadsingle then the number of numfiles needs to be greater than zero:

(a.assignmenttype <> 'uploadsingle' OR s.numfiles > 0)

Both of these must be true for a row to be included in the result. If I did an OR then an assignment of type upload would be included by the second part of the query weather valid or not and an assignment of type uploadsingle would be included by the first part (again valid or not).

You can test this for yourself if you wish by running the following query which will return unsubmitted upload and uploadsingle rows in assignment_submissions

   SELECT *
     FROM `mdl_assignment_submissions` s
LEFT JOIN `mdl_assignment` a ON a.id = s.assignment
    WHERE s.timemodified > 0 AND
          (a.assignmenttype <> 'upload' OR s.data2 = 'submitted') OR
          (a.assignmenttype <> 'uploadsingle' OR s.numfiles > 0);

The following code however will only return submitted rows and thus appears to fix this issue:

   SELECT *
     FROM `mdl_assignment_submissions` s
LEFT JOIN `mdl_assignment` a ON a.id = s.assignment
    WHERE s.timemodified > 0 AND
          (a.assignmenttype <> 'upload' OR s.data2 = 'submitted') AND
          (a.assignmenttype <> 'uploadsingle' OR s.numfiles > 0);

I was the reporter of the bug, so I only reported for master and I'm happy for it only to be fixed in master so that people actually have a good reason to upgrade. However if the integrators believe this should be backported then I will.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

I will do, thanks Eloy

Correcting branch name

I was asked to cherry-pick this back to the other branches mentioned in MDL-29400, so I have done so.

Finally apologies for not getting this done sooner, I had hoped to get this done before last week's integration but unfortunately I was not able gain remote access to my workstation.

The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

I'm moving out from these as integrator for now... will retake them only if I'm able to review properly all the "enrol" pending ones that have max priority for this week.

So any other integrator, feel free to pick them if pending... TIA and ciao

Hi Gerry,

I am reopening this issue for the same reason as I reopened MDL-30957.
Those hard coded assignment types should be avoided at all costs and chance are a similar solution could be found for this issue.
A quick grep shows the single use of assignment_count_real_submissions is assignment_base::count_real_submissions which is likely an extends assignment type class during the call and should be able to be overridden there.

Cheers
Sam

nods I was planning on coming back to this one after your comments in 30957.

@Sam Instead of overriding assignment_base::count_real_submissions() directly, I've made it so that assignment_base::count_real_submissions() becomes a modification of assignment_count_real_submissions() which calls another method containing the SQL statement which can then be overridden by other assignment types without a lot of unnecessary code duplication.

overriding assignment_base::count_real_submissions() is the only place were assignment_count_real_submissions() is actually called in our code base so I considered depreciating it, but I decided against it as there is probably a good deal of external code which is using that function.

I'm putting this up for peer review and Andrew was already peer reviewer on this, however if you spot something please jump in.

The phpdocs for assignment_count_real_submissions() are incorrect. Both the description and its missing an @param.

It seems weird that both assignment_count_real_submissions and count_real_submissions exist independently to do the same thing in a different way. Can we just get rid of count_real_submissions or make one call the other? Or am I missing something?

I think I've been misunderstood.

My code does not change assignment_count_real_submissions(), it changes assingment_base::count_real_submissions();

@Andrew

I've altered assignment_count_real_submissions() to call count_real_submissions() as suggested and updated it's doc block.

It would be great if you could take another quick look at this. Thanks for your input, it's appreciated.

The phpdocs comment isnt quite right for some of the implementations of count_real_submissions_select(). "This function should be overloaded by assignment types if they have different requirements for what constitues a complete submission." is correct for the parent classes. For the child classes you probably want to say something like "this overrides assignment::count_real_submissions_select() due to special requirements as to what constitutes a complete submission." Something like that that tells you whether youre looking at a function that is going to be overriden or one of the overriding functions.

Otherwise, its good to go

update: its up to you but you may also like to only create a master version until after peer review. It saves fiddling with multiple branches until you're about to submit for integration.

Sorry, one more thing. Add to the testing instructions to include testing each assignment type you've modified. Also have the test include both not submitting a file and actual submission just to check nothing accidentally got broken along the way.

Oops thanks Andrew. Nice catch

OK made the doc changes requested, submitting for integration. Will make the test changes in a moment.

Test instructions updated.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

rebased

Hi Gerry,

Thanks for splitting out the logic to the assignment type classes, it is really much better than hard coding.
Unfortunately there is still a few changes still required, below are my notes from the review:

1. assignment_count_real_submissions should not use assignment base. In order to return the correct count the function would have to initialise the correct object for the assignment type.
2. Actually @param on that function need to be reviewed as well.
3. count_real_submissions_select is an interesting one, at the moment I don't like how that function operates, in reality it is doing exactly what count_real_submissions should be doing. I can see why you have split it out as essentially the SQL is the only thing that really differs between classes however I think the separation could be further improved.
   I'd suggest the following:
   • Change the function to count_real_submissions_sql and have it operate like moodle_database::get_in_or_equal and have it return an array containing two parts the SQL and an array of params.
   • Pass the array of user id's to the new function rather than imploding it first. This is required because of the next point
   • You need to use $DB->get_in_or_equal when constructing an in statement as it differs across DB engines.
   • Make the new method protected. All new methods MUST define their scope.

Cheers
Sam

I'm quote of an IM discussion between myself and Sam regarding this issue, so that it can be continued here...

Gerry wrote:

I get your point about using get_in_or_equal(), I absolutely should be doing that. However transforming count_real_submissions_select() into count_real_submissions_sql() and returning an SQL and params array, while consistent with the rest of the Moodle code base, I feel is only making the logic more complex than it needs to be. Perhaps I didn't take in your entire reasoning in the scrum? If you say this is the right way to do it then I'll do as you request, but I just want to fully understand the reasoning.

Sam wrote:

I suppose the biggest thing is that I don't think the split is very clear
count_real_submissions_select is returning the count of the real submissions exactly what I would expect count_real_submissions to be doing, however that is now only processing logic to produce an array of users. So neither count_real_submissions nor count_real_submissions_select are doing what they suggest their names suggest they are doing.
I think the split needs to be clarified
There would be two ways to do it I imagine

1. Split out the logic that works out the users to select for into a function like get_potential_users and then having count_real_submissions doing the work count_real_submissions_select is doing presently
2. Split it as I suggested in the bug whereby we get a _sql function for generating SQL that count_real_submissions can use to perform the actual count

(noting that in 1. count_real_submissions would call the new get users function)
As you mentioned in scrum the solution have works why not use it?
Well that is still an option
Under normal circumstance we would strongly stick with changing to an approach that is consistent with Moodle and follows the coding guidelines in this case in regards to how functions are named, there have been many an issue drawn out while we debate what the best naming scheme is, or how best to structure function/methods as we are doing here
Having that consistency helps other developers pick up on what code is doing and provide a great starting place when looking for something in code you are unfamiliar with
Plus its very hard later on to change our minds and refactor code renaming functions etc as we don't change API in stable branches.
On the flip side the assignment module is due to be refactored in 2.3, and as part of that subtypes will be removed.
Perhaps that is reason enough to stick with your solution that works
So by all means I am open to being swayed to integrate it
Perhaps as an idea it would be worth talking to Martin to make sure the assignment refactor is on schedule to land for 2.3
If not then certainly I'd like to still see this refactored to sort out the division
Again by all mean though it is open for discussion, feel free to comment on the issue
[...] feel free to quote me on the issue in your reply etc [...]

Firstly I should state that everything I say here I am not saying for my own benefit, even if that is not currently clear.

I am in complete agreement with your first idea of how to split things up and this is exactly the goal I had, 1 function = 1 task is a great coding standard as far as I'm concerned. However I'm trying to achieve it in a piecemeal fashion as I believe this is more doable. It is my opinion that if a developer makes an incremental improvement, as long as they are not making anything worse it should be integrated even if it doesn't improve everything that the changes touches. My original patch while not perfect was an improvement (except for the param issue and non use of protected on the new methods). Point 1 was a problem that existed before the fix and the function is not called anywhere in the code base. Point 3.3 is also an issue that existed prior to the fix. Fixing these issues leads to a better solution:
https://github.com/gerrywastaken/moodle/compare/master...working_improvement-m_MDL-30724_count_real_submissions

Splitting out other logic into a get_potential_users() method is perhaps a further improvement
https://github.com/gerrywastaken/moodle/compare/master...mockup_of_suggestion-m_MDL-30724_count_real_submissions

Making that get_potential_users() generic so it can be used in other places would be better still, however I could also break the get_potential_users() down even further, rewrite other functions, the entire file, the assignment library or even most of Moodle given enough time. The point is there is always more that could be improved by a patch and how does a developer judge what an peer reviewer/integrator will consider perfect? If a developer fixes something extra and then we expect them to address other related issues, then they are probably less likely to do any extra work the next time. Instead we will just encourage one line fixes that just address the issue and doesn't attempt to improve the code base. This is why I think the goal for a pass should be a fix for the issue. Anything extra should be just considered a bonus as long as it makes things better than the current code. I believe it shouldn't have to be perfect.

Please don't take this a me trying to convince you to pass this issue or having a go at you. This is all just my opinion on what the policy should be in order to achieve the greatest good for Moodle in the long run.

Hi Gerry,

I've had a big discussion about this issue with Eloy this morning.
We were focusing on both your concerns about accepting the patch and what the best solution is for the issue.

First up in regards to requiring the perfect solution, by no means do we require the perfect solution, however our goal is to have a good final solution that adheres to the coding guidelines, fixes the issue at hand, and does so in a manner that fits with the rest of our code.
My job, or more my goal as an integrator is to help you in achieving that solution, I'm not here to test it, I am here to review the code you are producing taking into account not only the functionality of the code but also the relation between the problem and solution, the overall implications of the purposed solution and how it will work with the rest of our code as well as the code others may have developed to work with Moodle, and to consider how the solution fits within the goals and ideals we have for Moodle.
Yes, the reality is that this leads to issues being sent back each integration cycle so that it can be further worked on and discussed, and it definitely holds up the rate at which we fix issues. However I certainly feel Moodle is better for it.
I suppose the other thing to consider is that each week we are sending back around 20% of issues, I don't think in reality that number if very high, and certainly since we enacted the integration process some time back that number has got significantly lower.

As for the solution to this issue... I discussed this thoroughly with Eloy as mentioned, he picked up on a couple more things that I had missed and came up with a couple of great ideas.
After much discussion we came to the conclusion that the best solution would be in fact to have a single function in assignment types classes count_real_submissions and have each assignment type override the function in its entirety if required.
Presently that function is dedicated to the working out which users to search, Eloy being very clever in fact noticed that we have a function get_enrolled_sql($context, $withcapability, $groupid) that could be used in place of that logic to produce SQL and params that could be used in a nested query to get the users. This would greatly reduce the function size and of course reduce the number of database queries.
We decided this one function method would be the way to go because factoring in the use of get_enrolled_sql the function would be much shorter and that having additional functions wouldn't provide anything more interesting.
My apologies for the rewind here, such is the fun of having several people look at an issue after development has already started.

Cheers
Sam

First up in regards to requiring the perfect solution, by no means do we require the perfect solution, however our goal is to have a good final solution that adheres to the coding guidelines, fixes the issue at hand, and does so in a manner that fits with the rest of our code.
My job, or more my goal as an integrator is to help you in achieving that solution, I'm not here to test it, I am here to review the code you are producing taking into account not only the functionality of the code but also the relation between the problem and solution, the overall implications of the purposed solution and how it will work with the rest of our code as well as the code others may have developed to work with Moodle, and to consider how the solution fits within the goals and ideals we have for Moodle.
Yes, the reality is that this leads to issues being sent back each integration cycle so that it can be further worked on and discussed, and it definitely holds up the rate at which we fix issues. However I certainly feel Moodle is better for it.
I suppose the other thing to consider is that each week we are sending back around 20% of issues, I don't think in reality that number if very high, and certainly since we enacted the integration process some time back that number has got significantly lower.

As for the issue, thanks for discussing it further and it doesn't bother me at all that you have come up with an alternative solution. I'll look into your ideas and resubmit. Thanks Sam (and Eloy)

@Sam (or Eloy)

Is this more of less what you had in mind?
https://github.com/gerrywastaken/moodle/compare/master...m_MDL-30724_count_real_submissions_eloy_suggests

Hi Gerry,

The assignment class methods look spot on thank you.
Just noting one thing the hard coding within assignment_count_real_submissions would be great to avoid as there may be people out there who want to fix there assignment types, or who have extended the existing assignment types and will benefit from this fix.
How about something like the following?

$type = $DB->get_field($cm->modname, 'assignmenttype', array('id' => $cm->instance));
$file = "{$CFG->dirroot}/mod/assignment/type/{$type}/assignment.class.php";
$class = "assignment_{$type}";
if (file_exists($file) && !class_exists($class)) {
    require_once($file);
}
if (!class_exists($class)) {
    $class = 'assignment_base';
}
$instance = new $class;
$instance->cm = $cm;
return $instance->count_real_submissions($groupid);

Cheers
Sam

nods very good point and I love the code, much better than what I was doing. Thanks Sam.

Requested changes made. Thanks Sam, Eloy and Andrew.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Woohoo this has been integrated now, congrats Gerry!

Thanks Sam and everybody else.

Test result: Success. The count appears correctly all around. I discovered that when a student clicks save without any files, it still appears in the submissions table as submitted. I've commented on MDL-29400 about this.

Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!).

icao_reverse('arreis olik rebemevon afla letoh ognat');

Closing, ciao

We have just noticed that if an assignment has already received submissions and the allowed submission dates for the assignment are then changed so that the submitted assignments are not in the allowed time frame the assignment will show a link that says "No submitted assignments" when there are some. Is this the correct outcome? It seems that there need to be some processes to keep this from happening.

What would be the proper course of action?
1. Should the assignments that are submitted previous to a time allowed have their submission date automatically changed to be inside the time frame
2. Should previously submitted assignments be deleted and a notice be sent to the students?
3. Should the teacher be unable to change the assignment dates once an assignment has been submitted?

Any other ideas?

We updated to the latest version of Moodle that has this fix. Several teachers have reported their assignments now say that that "no attempts have been made on this assignment"

When you click that link that says no attempts, you see the assignment submissions page and there are indeed several assignment submissions.

We rolled back to the old code from the previous week's release:
assignment/lib.php
assignment/type/upload/assignment.class.php
assignment/type/uploadsingle/assignment.class.php

and the assignments list the number of attempts correctly.

Discovered our real issue is MDL-31221 causing a count of 0 to be returned because data2 is not "submitted" since it is never officially "Sent for Marking".

Hi, Guys.

I thought I should add a note that no one is watching this issue now it is closed.

Please check out the linked issues and if you feel there is still a problem, please launch a new issue.