Fixed: Can set earlier due date as an individual due date extension and related bugs

[bradenmacdonald]

This pull request fixes LMS-6563: Can enter in earlier date in Individual Extension, which was also mentioned in the individual due date extensions feature PR #2062.

Now an error will occur if the instructor attempts to set a due date that is earlier than the original due date.

I also fixed two related bugs that I noticed while working on this:

• The API would allow setting a due date extension on a unit that had no due date at all. This now returns an error.
• If one set an individual due date extension, then deleted the original due date, an Internal Server Error (500) would occur when deleting the due date extension, and neither a success nor failure message would be shown to the user.

Unit tests are included.

This was done for OpenCraft ( cc @antoviaque ) and falls under their organization contributor agreement.

[sarina]

@bradenmacdonald thanks for the contribution! I have run a build on your branch. I can take a pass through this later today.

@carsongee or @chrisrossi would either of you be able to review this PR?

[sarina]

@bradenmacdonald once you have a passing build, please click on the green check next to your commit to view the output of your build:

From there, please view the "Diff Coverage" and "Diff Quality" reports:

The goal is to have as close to 100% of lines covered on the diff coverage report (which measures which lines of code are covered by tests). Some lines are not possible or very hard to test, but please do check and add additional tests if appropriate.

The "Diff Quality" report should always be 100%; there is no reason to have it be less. You can generate this report locally by running paver run_quality (make sure you run this from the edx-platform directory so you pick up our .pep8 and pylintrc files); see https://github.com/edx/edx-platform/wiki/Python-Guidelines for general Python style guidelines in the platform.

[sarina]

Style nit: splitting across lines should look like this:

self.assertEqual(
    None,
    get_extended_due(self.course, self.week3, self.user1)
)

[sarina]

@bradenmacdonald would you have the bandwidth to fix one more bug with this feature? When this is enabled and I click on the "Extensions" tab on the Instructor dashboard, I get jumped down to the VIEWING GRANTED EXTENSIONS section. This is weird, and this jumping shouldn't happen. Instead when I click the "extensions" tab, I should stay on the same place on the page (just like every other tab).

[sarina]

Another bug I find is as follows: when a username is entered that is not a real username, nothing in the UI indicates this - nothing says there was a failure, no error message is displayed. Contrast this with the behavior of, eg, the student admin tab:

These two bugs would be great follow-on tasks to this PR. I would not try to tackle these bugs in this same PR - we prefer to have many small PRs rather than one large one.

👍 on this current PR as-is, once my few comments are addressed.

[carsongee]

Thanks for the help, this one has been on my to do list for too long. I also agree with Sarina's additional issues. Fixing the weird anchor jump would be very nice.

[bradenmacdonald]

Thanks a lot for reviewing, @sarina and @carsongee . I can definitely fix those other two issues you've pointed out :)

[sarina]

@bradenmacdonald you will need to perform a rebase on this PR as it has a conflict with master.

[bradenmacdonald]

Ok, I believe I have addressed all the comments made so far:

• Added missing unit test. Has 100% coverage now.
• Will now display error when attempting to reset a nonexistent extension (commit 9c93cd8)
• Minor: Clarified message that said "(no due date)"
• Minor: Fixed formatting of split line method calls in unit tests
• Minor: Squashed and rebased onto latest master (only conflict was AUTHORS file)

I also have fixes for the other two issues mentioned by @sarina - I will open separate pull requests for them.

[sarina]

@bradenmacdonald did you figure out the issue you were having with your test?

[bradenmacdonald]

@sarina Yes, I did :) The test in question just needed this line:

self.week1 = self.store.update_item(self.week1, self.user1.id)

I put all the unit tests as the first commit in this branch so it's easy to checkout the tests without the fixes as a git detached head. I confirmed that this test (and the others I've added) does not pass without the fix included in this PR.

[carsongee]

This looks good to me. Thanks again

[sarina]

LGTM as well. Thanks for the contribution!