Moodle
  1. Moodle
  2. MDL-30724

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

    Details

    • Testing Instructions:
      Hide
      1. Login as Admin
      2. Create a new assignment of type "advanced file upload"
        1. Click "Upload file"
        2. Add some files
        3. "Save changes"
        4. Look at the links under "Settings > Assignment administration" and observe that there is one that reads "No attempts have been made on this assignment"
        5. Click "Send for marking"
        6. Click "Continue"
        7. Look at the links under "Settings > Assignment administration" and observe that there is one that reads "View 1 submitted assignments"
      3. Create a new assignment of type "simple file upload"
        1. Click "Upload a file"
        2. Click "Save changes" without adding any files
        3. Look at the links under "Settings > Assignment administration" and observe that there is one that reads "No attempts have been made on this assignment"
        4. Click "Upload a file" again
        5. Add a file
        6. Click "Save changes"
        7. Look at the links under "Settings > Assignment administration" and observe that there is one that reads "View 1 submitted assignments"
      Show
      Login as Admin Create a new assignment of type "advanced file upload" Click "Upload file" Add some files "Save changes" Look at the links under "Settings > Assignment administration" and observe that there is one that reads "No attempts have been made on this assignment" Click "Send for marking" Click "Continue" Look at the links under "Settings > Assignment administration" and observe that there is one that reads "View 1 submitted assignments" Create a new assignment of type "simple file upload" Click "Upload a file" Click "Save changes" without adding any files Look at the links under "Settings > Assignment administration" and observe that there is one that reads "No attempts have been made on this assignment" Click "Upload a file" again Add a file Click "Save changes" Look at the links under "Settings > Assignment administration" and observe that there is one that reads "View 1 submitted assignments"
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      m_MDL-30724_count_real_submissions_eloy_suggests
    • Rank:
      33600

      Description

      The Advanced uploading and single file upload assignments both will create an entry in the assignment_submissions table as placeholders. The assignment_count_real_submissions() function counts these as valid submissions when it should not.

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment - - edited
          (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.

          Show
          Andrew Davis added a comment - - edited (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.
          Hide
          Gerard Caulfield added a comment -

          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);
          
          Show
          Gerard Caulfield added a comment - 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);
          Hide
          Gerard Caulfield added a comment -

          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.

          Show
          Gerard Caulfield added a comment - 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.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Gerard Caulfield added a comment -

          I will do, thanks Eloy

          Show
          Gerard Caulfield added a comment - I will do, thanks Eloy
          Hide
          Gerard Caulfield added a comment -

          Correcting branch name

          Show
          Gerard Caulfield added a comment - Correcting branch name
          Hide
          Gerard Caulfield added a comment -

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

          Show
          Gerard Caulfield added a comment - I was asked to cherry-pick this back to the other branches mentioned in MDL-29400 , so I have done so.
          Hide
          Gerard Caulfield added a comment -

          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.

          Show
          Gerard Caulfield added a comment - 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.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Gerard Caulfield added a comment -

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

          Show
          Gerard Caulfield added a comment - nods I was planning on coming back to this one after your comments in 30957.
          Hide
          Gerard Caulfield added a comment -

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

          Show
          Gerard Caulfield added a comment - @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.
          Hide
          Andrew Davis added a comment - - edited

          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?

          Show
          Andrew Davis added a comment - - edited 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?
          Hide
          Gerard Caulfield added a comment -

          I think I've been misunderstood.

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

          Show
          Gerard Caulfield added a comment - I think I've been misunderstood. My code does not change assignment_count_real_submissions(), it changes assingment_base::count_real_submissions();
          Hide
          Gerard Caulfield added a comment - - edited

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

          Show
          Gerard Caulfield added a comment - - edited @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.
          Hide
          Andrew Davis added a comment - - edited

          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.

          Show
          Andrew Davis added a comment - - edited 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.
          Hide
          Andrew Davis added a comment -

          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.

          Show
          Andrew Davis added a comment - 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.
          Hide
          Gerard Caulfield added a comment -

          Oops thanks Andrew. Nice catch

          Show
          Gerard Caulfield added a comment - Oops thanks Andrew. Nice catch
          Hide
          Gerard Caulfield added a comment -

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

          Show
          Gerard Caulfield added a comment - OK made the doc changes requested, submitting for integration. Will make the test changes in a moment.
          Hide
          Gerard Caulfield added a comment -

          Test instructions updated.

          Show
          Gerard Caulfield added a comment - Test instructions updated.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Gerard Caulfield added a comment -

          rebased

          Show
          Gerard Caulfield added a comment - rebased
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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: 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. Actually @param on that function need to be reviewed as well. 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
          Hide
          Gerard Caulfield added a comment -

          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.

          Show
          Gerard Caulfield added a comment - 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 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 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.
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Gerard Caulfield added a comment -

          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.

          I understood most of this before writing my comments and I would also have expected the number getting sent back to get smaller, but for different reasons. Thanks for listening and responding I'll leave this conversion at what I said in my previous message.

          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)

          Show
          Gerard Caulfield added a comment - 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. I understood most of this before writing my comments and I would also have expected the number getting sent back to get smaller, but for different reasons. Thanks for listening and responding I'll leave this conversion at what I said in my previous message. 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)
          Hide
          Gerard Caulfield added a comment -
          Show
          Gerard Caulfield added a comment - @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
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Gerard Caulfield added a comment -

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

          Show
          Gerard Caulfield added a comment - nods very good point and I love the code, much better than what I was doing. Thanks Sam.
          Hide
          Gerard Caulfield added a comment -

          Requested changes made. Thanks Sam, Eloy and Andrew.

          Show
          Gerard Caulfield added a comment - Requested changes made. Thanks Sam, Eloy and Andrew.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Sam Hemelryk added a comment -

          Woohoo this has been integrated now, congrats Gerry!

          Show
          Sam Hemelryk added a comment - Woohoo this has been integrated now, congrats Gerry!
          Hide
          Gerard Caulfield added a comment -

          Thanks Sam and everybody else.

          Show
          Gerard Caulfield added a comment - Thanks Sam and everybody else.
          Hide
          Michael de Raadt added a comment -

          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.

          Show
          Michael de Raadt added a comment - 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.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Matthew Davidson added a comment -

          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?

          Show
          Matthew Davidson added a comment - 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?
          Hide
          Ryan Smith added a comment -

          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.

          Show
          Ryan Smith added a comment - 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.
          Hide
          Matthew Davidson added a comment -

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

          Show
          Matthew Davidson added a comment - 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".
          Hide
          Michael de Raadt added a comment -

          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.

          Show
          Michael de Raadt added a comment - 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.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: