Fix setting user overrides for dates on the user detail page for set versions.

[drgrice1]

This addresses issue #2340.

Currently if an instructor unchecks a checkbox for a user's versioned set on the user detail page (the page you can reach by clicking on the "Assigned Sets" column for a user in the Accounts Manager), then when dates are checked (in the checkDates method of UserDetail.pm), the dates from the global set record are used. If those are valid dates (they usually will be in this case), then those valid dates are returned with no error. Thus the database will be updated. However, the logic used to determine how the database is updated does not match the logic used to check the sets, and since the override checkbox is not checked, the date in the user's versioned set record is set to NULL. So for versioned sets this makes it so that if the checkbox is not checked or the override date is not set, then the global set date is NOT used for a fallback. Instead 0 is used for a fallback. Since that is not a valid date, this forces an error, and the database is not updated.

This is actually a more general issue with the logic used when the dates are checked and the logic used when the database is updated not being the same. So that is fixed.

Additionally when the dates in the database are updated, there are cases where the user interface does not correctly reflect the changes made. One such case is for a regular set if the checkbox for a date is checked but the override date left empty. Then the global set date is used (meaning the user set value for that date is set to NULL in the database), however the checkbox remains checked. So the corresponding parameters are updated so that the changes are reflected when the page rerenders after saving.

Note that in the case that the dates are not updated in the database due to an error in the given dates, the user interface does not update to reflect the current database state. I don't think the user interface should be updated in this case, as the instructor might want an opportunity to correct the error and not have their changes wiped out.

I also noticed that the "User overrides" header was being displayed over the "Open", "Closes", "Answer" labels in the case that a set is not assigned to a user (and so there are no override fields). So that is not shown in that case anymore.

I also removed the 10 year limit on dates. I am not sure why we impose this limit. There is no reason to do so. In fact, it is quite useful in many situations to have a due date that is more than 10 years in the future. By the way, I was curious if these dates were affected by the year 2038 problem. So after removing this restriction I set a due date to be in 2050. This seems to work with no issue. So it seems that 64bit integers are being used in this case which are not affected by the year 2038 problem. So we have 292 million years before this will be an issue again.

Edit: Additional changes added.

The header on the user override dates now read "User Overrides" unless it is a test version in which case it is "User's Test Version Dates".

The header on the dates for the global set reads "Test Dates" for tests and "Assignment Dates" for other assignments.

The placeholder in the user override date inputs now reads "Required" for test versions (except for the reduced scoring date which reads "Test Default"), "Test Default" for test template sets, and "Assignment Default" for other assignments. Furthermore, for test versions the dates have the required attribute (except for the reduced scoring date), so the form will not submit unless those dates are provided. Note that the reduced scoring date works differently than the other dates, and is not filled in when the test is submitted like the other dates are.

The date override check boxes have been removed.

Remove the "s" from the end of the "Close" date label. That was inconsistent with the other date labels.

The class dates for the set are now in readonly inputs instead of just being text. This is the same as on the set detail page. One advantage of this is that it makes it easier to cut and paste those dates.

There is also a small tweak to the datepicker.js (which is used on this page as well as on the sets manager page and the set detail page). When flatpickr adds the date picker, it hides the input originally on the page, and adds another input. This moves the id from the original input onto the added flatpickr input so that labels still work. The ids aren't used for anything else after the date picker javascript has found them. Also remove the placeholder that flatpickr copies to the new input since that isn't valid on a hidden input.

[drgrice1]

Sounds good to rename them. How about "Version overrides" instead of "Test version for user" though?

[drgrice1]

Or "User version values", or "Test version values"? I guess in a sense these are not exactly overrides. They are in terms of how the database works, but they aren't in the sense that it doesn't make sense for a test version to fill in the parent test values for these. In fact, would it be a good idea to even drop the "Parent test values" (or "Set values") part of the dates table for a test version?

[drgrice1]

Another way to go that might clarify things in general would be to not use the terminology "Set values" anywhere. Instead use "Course values". Also, maybe "values" is wrong entirely. These are all "dates", so why not just say so. So use "Course dates" instead.

So regular sets and test template sets would have "User overrides" and "Course dates", and test versions would only show the user dates with the header "Test version dates".

How does that sound?

[drgrice1]

So taking the further suggesting further, if we go that direction and remove the course dates from test versions, then the override check boxes would also be removed. With this pull request, they can't effectively be unchecked anyway. If they are unchecked the save of the dates is refused. So there is no point in them being there.

[pstaabp]

So regular sets and test template sets would have "User overrides" and "Course dates", and test versions would only show the user dates with the header "Test version dates".

This sounds clearer than the vague term "values".

[drgrice1]

Here is a screenshot after making the changes I suggested above. This shows all of the possibilities. The first set is a regular homework set, the second is a test template set, the third a test version, and the fourth a set that is not assigned to this user. If this sounds good, then I will add it to this pull request.

[Alex-Jordan]

I think these are all good. More thoughts I have:

• "Assignment dates" instead of "Course dates" on a homework set
• "Test dates" instead of "Course dates" on a test template set
• "User's test version dates" instead of "Test version dates"
• Capitalize both words in the headers
• aligning the date fields, even in the test version row to align with the other rows
• Do we need checkmarks ever? Imagine there are no checkmarks. If the use saves and the field is empty, that could match the current behavior of unchecking. Or if the user saves and a field is nonempty, that should imply the item is checked.

I don't feel strongly about any of these if you don't like them.

[drgrice1]

* "Assignment dates" instead of "Course dates" on a homework set
* "Test dates" instead of "Course dates" on a test template set
* "User's test version dates" instead of "Test version dates"

I am fine with these. I think "User's Test Version Dates" is a bit long, but it seems to fit okay, I guess.

* Capitalize both words in the headers

I agree on this.

* aligning the date fields, even in the test version row to align with the other rows

I realized that I went to far in my screenshot, and removed the date labels ("Open", "Close", etc.). That will obviously be added back, and the helps to line this up a little more. With those added back, if the check boxes are also removed in general, then this will just line up without further effort.

* Do we need checkmarks _ever_? Imagine there are no checkmarks. If the use saves and the field is empty, that could match the current behavior of unchecking. Or if the user saves and a field is nonempty, that should imply the item is checked.

I am okay with this, but it does seem that it makes it less clear how to remove a date override to make this change. Perhaps I am wrong on this? Note, that this request will take a rather heavy rewrite of the module in general though.

[Alex-Jordan]

Note, that this request will take a rather heavy rewrite of the module in general though.

Let's forget about it for now. If that change is made, it should be consistent everywhere in other places that have these checkboxes.

I've never understood why they are needed though. Fields are on the screen. A user can change what is in those fields. With some systems, that is all that is needed and there is not even a need for a save button. But forget about that, we will keep the save button. So then you save, and whatever is in the fields at that time is saved (if it validates).

[drgrice1]

I've never understood why they are needed though. Fields are on the screen. A user can change what is in those fields. With some systems, that is all that is needed and there is not even a need for a save button. But forget about that, we will keep the save button. So then you save, and whatever is in the fields at that time is saved (if it validates).

This page is designed to be like the dates on the "Set Manager" page. On that page there are cases where the check boxes are necessary. Namely when the override field is a drop down menu. Although, now that I write that I realize that you could add a "use set default" option to the select in that case.

For this page, it would not be that hard to remove the check boxes. Also, the only way to get nice alignment of dates for versioned sets with out check boxes with the other sets is to also not have check boxes for the other sets.

By the way, not having a save button is not a good idea in my opinion. The systems that do that are sending a network request each time the input changes to save the data to the server. That results in excessive network traffic. A more important reason though is just the ease of reverting if you decide you don't like the changes. For a page like this that is useful. You can just reload the page, and get the current settings in the database back. Most systems that I have seen similar to this have a save button for that reason (Canvas, Moodle, Pearson MyLab, and Web Assign all have save buttons for pages where you mass edit assignment dates or settings).

[Alex-Jordan]

This page is designed to be like the dates on the "Set Manager" page.

Do you mean the Set Details page for a user (or subset of users)? That's what I was thinking when I mentioned a change like that should happen in other places. Those dropdowns could have "None Specified" as an option, which is the placeholder text in the neighboring text fields.

If there are location restrictions, then "Restrict Locations" is a multi-select that appears there. I can't seem to unselect something there so either things would need to be made "unselectable" there or it would need "use set default"/"None Specified" as well.

I agree about save buttons. I feel unsure of myself when they are not there.

[drgrice1]

It seems to me that the placeholders for the text inputs on the "Set Detail" page when editing for a user would be more clear if they read "Set Defaults" (or "Assignment Defaults"). or something that indicates what it means if the input is left blank. Particularly if the check boxes are removed.

[drgrice1]

I implemented the changes we discussed. Removing the check boxes wasn't as big of a rewrite as I though, so I did it. Make it a TODO to do the same (in another pull request) for the set detail page.

[Alex-Jordan]

if they read "Set Defaults" (or "Assignment Defaults").

That sounds good.

On Thu, Feb 29, 2024, 5:16 PM Glenn Rice @.***> wrote:

It seems to me that the placeholders for the text inputs on the "Set Detail" page when editing for a user would be more clear if they read "Set Defaults" (or "Assignment Defaults"). or something that indicates what it means if the input is left blank. Particularly if the check boxes are removed.

— Reply to this email directly, view it on GitHub https://protect2.fireeye.com/v1/url?k=31323334-501d2dca-3132feb7-454455534531-10d2afc0d7d97e57&q=1&e=4c08192a-6e50-4955-8ac4-644914dece12&u=https%3A%2F%2Fgithub.com%2Fopenwebwork%2Fwebwork2%2Fpull%2F2345%23issuecomment-1972268074, or unsubscribe https://protect2.fireeye.com/v1/url?k=31323334-501d2dca-3132feb7-454455534531-d29626799568bb46&q=1&e=4c08192a-6e50-4955-8ac4-644914dece12&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABEDOAFYFGOEZQCDKJ6NHC3YV7JGRAVCNFSM6AAAAABD4GHPGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZSGI3DQMBXGQ . You are receiving this because you commented.Message ID: @.***>

[Alex-Jordan]

This looks good, but I started playing with things and it led to a PR I just opened to your branch.

[drgrice1]

There is a downside of removing the check boxes. If an instructor wants to remove all override dates for a student, then with the check boxes all that needs to be done is to uncheck all check boxes. Without the check boxes the values in all of the inputs have to be deleted which does take more work. Admittedly removing override dates is something that generally shouldn't be done. If you have extended the due date on an assignment, then removing that extension would be bad practice.

[drgrice1]

There is bad news for removing check boxes on the problem set detail page when editing a set for a user. I am not saying that it can not be done, but it will be quite challenging and will take quite a bit of work. The most challenging part will be dealing with test versions. To make it all work it really will take a rather hefty rewrite of that module, and much care will be needed to get it right.

[Alex-Jordan]

OK, I regret making suggestions that take this so far astray from the bugfix. What do you think is best to tie a bow on this PR? Revert the checkboxes and punt the other things in that PR I opened?

[drgrice1]

I think it is fine to continue on the path of this pull request and what we have done. This page does not have to be entirely consistent with the set detail page. Eventually, it would be nice to get rid of the check boxes there too. It will clean up the page quite a bit. Everything is so tight because so much is squeezed into that page, and getting rid of an unnecessary UI element is helpful for that.

[drgrice1]

I added translation of the check date error messages and a few other tweaks.

[Alex-Jordan]

I checked it out and tested, and it's all good. Consider my approval renewed!

I'll let @pstaabp (or anyone) take a look again before merging though.

[pstaabp]

All of these are great additions. The help box clarifies the issued that this started from and I think the label of dates are much clearer now.

[drgrice1]

I made one last minor change. I didn't like that if the open, close, or answer date is not set it says Set [_1] has errors in its dates: open_date |[_2]|, due date |[_3]|, answer_date |[_4]|, where the dates shown are the unix timestamps. So now those are formatted dates if set, and "required" if not. Also, I changed "due date" to "close date" in the messages, since that is what this date is labeled in the user interface.

[Alex-Jordan]

Since @pstaabp gave this another look, I'm going to proceed with merging it.