[Feature:InstructorUI] TA Beta Testing Date Constraints#12417
Conversation
Added a warning description for when the ta beta testing date is after the submission open date for students.
Spent about 2 hours trying to learn how migrations work on Submitty and ended up removing what appears to be the TA view (beta testing) constraint from the Gradeable table. This should hopefully prevent constraint issue when choosing dates.
Removed these PHP constraints for all gradeable types.
Fixed an issue where the ta view date could not be after the team lock date.
Fixed ordering for both: Warning messages when setting gradeable dates. Gradeable categorization when ta view date was after the present and open submission date was before the present.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12417 +/- ##
============================================
- Coverage 21.66% 21.65% -0.01%
- Complexity 9638 9649 +11
============================================
Files 268 268
Lines 36223 36228 +5
Branches 486 486
============================================
- Hits 7847 7846 -1
- Misses 27894 27900 +6
Partials 482 482
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Christian2147
changed the title
github-project-automation
Bot
moved this from Seeking Reviewer
to Work in Progress
in Submitty Development
This reverts unnecessary DB Dump changes.
Fixed ta view date and submission open date warning popping up when submission open date is not visible.
JManion32
left a comment
JManion32
left a comment
There was a problem hiding this comment.
I noticed when I set the TA Beta testing Date Constraint to the end of time on a gradeable in the "FUTURE" section and then click OPEN TO TAS NOW, the gradeable jumps to the bottom of the page in the "Grades Available" section. Is this intended functionality since it would compare all of the dates and land on "Grades Available" since it is supposed to be the last one in chronological order?
I'd also like to know more about automated testing. The gradeables page is critical to courses, and gradeable date constraints have been reported to cause crashes (see #12204 and #10874). Additionally, there is a lot of edge cases that would be hard to adequately test manually.
Have you checked to see what E2E tests we currently have for changing gradeable dates? There appears to already be some in edit_gradeable.spec.js. It is definitely worth expanding on whatever exists now so that you have a nice foundation for this PR and all follow-ups for other constraints.
|
Ive added some specifically for grade inquries when I first made the first remove constraints PR. These definietely need more E2e tests, but I haven't been looking at this since CI is failing |
After some testing turns out this button just does this regardless of what you set the dates to. I will look into this today. |
This button would previously set all dates to now. Made it so that it only changed the TA view date to now.
Fixed a test that should show all changes saved when TA view date is after submission open date. Also added an additional test that makes sure the gradable is visible to all when submission open date is in the past and ta view date is in the future.
Using testid instead of id for submission open date.
- removed this because it was already set to past date
- Need to log back in as instructor after testing gradeable visibility with students, graders, and Tas.
Fixed students not being able to view the gradeable if the ta view date was in the future and the submission open date in the past.
Fixed an issue where TAs could view FUTURE gradeables where the open submission date did not exist.
automateprojectmangement
Bot
moved this from Work in Progress
to In Review
in Submitty Development
All CIs should be fixed now. |
When ta beta testing date is after the manual grading date, a warning will now pop up.
Removed redundant operation in cypress testing
Finished lab gradeables cypress tests
Added check to make sure date_ta_view is accessible.
I removed this update dates because on github, it is already set to future date. I will remove this comment once it appears to be working properly.
dates on github cypress != dates on local cypress
made 'All Changes Saved' acceptable and '' acceptable
Manual grading in the past and TA viewing in the future means it is visible for graders, tas, and instructors.
automateprojectmangement
Bot
moved this from Work in Progress
to In Review
in Submitty Development
JManion32
left a comment
JManion32
left a comment
There was a problem hiding this comment.
The code looks good to me.
I ran the Cypress test twice locally. It worked the first time (though an unrelated test failed). The second time, the first two passed, and the rest, including the new one, failed. Not sure if this is what the results have been for you. I am on a fresh VM snapshot.
We need more functionality reviews here, and it would be nice to have that Cypress test working reliably locally, but that may not be feasible if other tests aren't being atomic.
I am honestly having trouble getting any of the cypress tests anywhere to pass consistently locally. But I know that on github, the edit-gradeable.spec tests are consistently passing, and I have ran them at least twice I believe. |
JManion32
left a comment
JManion32
left a comment
There was a problem hiding this comment.
The code and functionality look great. Please make sure to update the Automated Testing and Documentation portion of the description to talk about the Cypress test you added.
The only thing that is holding me back from approving is the unreliability of edit_gradeable.spec.js locally. As we verified in class on a fresh vagrant destroy vagrant up on main, nothing appears to be caused by your changes, since it even fails then. We just can't rule it out 100% since we can't test it consistently.
I think @williamjallen will be at the developer meeting today, and we should discuss this with him. Great work with this PR. Working with dates is complex, and there were a lot of weird cases to consider here.
aconfo
left a comment
aconfo
left a comment
There was a problem hiding this comment.
I performed a functionality review. Testing if a TA open date in the future, with submission open date in the past performed as intended and was still under the "OPEN" section.
With this assignment I tried reviewing it, adding submissions, closing submissions, without encountering any obvious errors with the functionality. I also tried to access a future gradeable as a student which it prevented, as well as a beta gradeable, which is also prevented me from editing. When changing the dates of a future assignment such that the TA date was still in the future, but the open submission date the past, It would be open, in the open section, and visible to both students and TA's. Ta's would be able to preview grading when it was open as well, and I couldnt notice any immediate errors again. Overall It looks pretty good, and I didn't find any immediate errors in my testing, there may of course be some errors that I missed or didn't test.
John-Roy123
left a comment
John-Roy123
left a comment
There was a problem hiding this comment.
I did a functionality test and all of the intended changes work with this PR, and I did not come across any bugs while testing.
JManion32
left a comment
JManion32
left a comment
There was a problem hiding this comment.
Great work with this. Though the edit_gradeable Cypress tests are inconsistent locally, we have verified that they pass CI consistently. This PR is a great template for eliminating the rest of the constraints in the future.
williamjallen
left a comment
williamjallen
left a comment
There was a problem hiding this comment.
Code looks good to me. As noted previously, the down migration is potentially risky. I'll leave the decision about whether that risk is acceptable to Barb.
github-project-automation
Bot
moved this from In Review
to Done
in Submitty Development
### CAUTION: Since this PR removes a date constraint, if it is installed on a production system and then needs to be reverted after a moderate period of time where gradeables might have been created/edited and taking advantage of the more flexible date settings, rolling back / reverting the change might fail. ### Why is this Change Important & Necessary? Partially addresses Submitty#12349 This PR specifically removes all constraints for the TA beta testing date for gradeables. This is important and necessary because instructors felt that the current UI for setting dates on gradeables was confusing, and they want to have more freedom over what dates they set for gradeables, even if they are invalid, since a lot of those dates may not matter for the specific class or assignment. ### What is the New Behavior? Now, the TA beta testing date has no constraints, it can be set to whatever date regardless of other dates. When one of the lower dates is set to before the present day and the TA beta testing date is set to the future, the lower date wins when assigning a gradeable status on the gradeables page. A date with actual constraints: <img width="2960" height="1755" alt="Constraint visible" src="https://github.com/user-attachments/assets/17cf85c4-cc63-4171-bee1-b1f7b37a0875" /> Previously, the TA beta testing date also had a constraint (where its box would turn red and changes would fail to save) if it was after any of the dates below it (For example: open submission date). Now, when the TA beta testing date is ahead of one of the dates below it, such as the open submission date (Only one that really matters now since all the other dates have constraints), it will display a message like the one shown here: <img width="3000" height="1891" alt="image" src="https://github.com/user-attachments/assets/b1c80060-bdc1-4e82-81a9-ce954a8aadfd" /> Additionally, when the TA beta testing date is in the future, and the open submission date is in the past, the open submission date wins and the assignment shows as open. This is also true for all dates below the TA viewing date. ### What steps should a reviewer take to reproduce or test the bug or new feature? Log in as instructor. Go to the sample course, try editing all gradeables and try changing their TA viewing date in all different combinations. Make sure that not only can the date be changed to any valid date, but also that the listing of the gradeable on the gradeables page is valid. For example, if open submission date is in the past, and TA viewing date in the future, then the assignment should show under OPEN, not under FUTURE, and not under BETA. Secondly, On the gradeable page, make sure all the buttons are visible and function properly. For example, if the assignment shows as OPEN, all appropriate buttons should be visible and should function like normal. Finally, go to a gradeable labeled FUTURE and a gradeable labeled BETA and try to copy the link. Then log out and log in as a student and try to paste the link. You should NOT be able to access the gradeable. If you can, please report it immediately. Note: While testing, the only dates that should have full free access to be changed without causing any constraint issues currently are the grade inquiry dates and the ta view date. The rest of the dates still have constraints on them. ### Automated Testing & Documentation <!-- Is this feature sufficiently tested by unit tests and end-to-end tests? If this PR does not add/update the necessary automated tests, write a new GitHub issue and link it below. Is this feature sufficiently documented on submitty.org? Link related PRs or new GitHub issue to update documentation. --> This feature is sufficiently tested by unit tests and cypress tests. Cypress tests have been added to ensure that the gradables are visible to the proper users when the TA beta testing date is changed along with the other dates, especially in ways that would previously violate a constraint. I have also added additional cypress tests for lab and other gradeables to ensure that the TA view date along with the other dates are working properly without the constraint. These tests also mainly consist of moving the dates around and checking to ensure the gradeable is viewed by the proper access levels. ### Other information <!-- Is this a breaking change? Does this PR include migrations to update existing installations? Are there security concerns with this PR? --> This is not a breakage change. However, this PR DID add a create a new migration for the course database. It removed any DB constraints from the gradeable table relating to g_ta_view_start_date. Link hijacking might be a security concern with this PR and should be tested. Ensure that ONLY instructors have access to FUTURE gradeables and instructors and TAs/Mentors have access to BETA gradeables.
### CAUTION: Since this PR removes a date constraint, if it is installed on a production system and then needs to be reverted after a moderate period of time where gradeables might have been created/edited and taking advantage of the more flexible date settings, rolling back / reverting the change might fail. ### Why is this Change Important & Necessary? Partially addresses Submitty#12349 This PR specifically removes all constraints for the TA beta testing date for gradeables. This is important and necessary because instructors felt that the current UI for setting dates on gradeables was confusing, and they want to have more freedom over what dates they set for gradeables, even if they are invalid, since a lot of those dates may not matter for the specific class or assignment. ### What is the New Behavior? Now, the TA beta testing date has no constraints, it can be set to whatever date regardless of other dates. When one of the lower dates is set to before the present day and the TA beta testing date is set to the future, the lower date wins when assigning a gradeable status on the gradeables page. A date with actual constraints: <img width="2960" height="1755" alt="Constraint visible" src="https://github.com/user-attachments/assets/17cf85c4-cc63-4171-bee1-b1f7b37a0875" /> Previously, the TA beta testing date also had a constraint (where its box would turn red and changes would fail to save) if it was after any of the dates below it (For example: open submission date). Now, when the TA beta testing date is ahead of one of the dates below it, such as the open submission date (Only one that really matters now since all the other dates have constraints), it will display a message like the one shown here: <img width="3000" height="1891" alt="image" src="https://github.com/user-attachments/assets/b1c80060-bdc1-4e82-81a9-ce954a8aadfd" /> Additionally, when the TA beta testing date is in the future, and the open submission date is in the past, the open submission date wins and the assignment shows as open. This is also true for all dates below the TA viewing date. ### What steps should a reviewer take to reproduce or test the bug or new feature? Log in as instructor. Go to the sample course, try editing all gradeables and try changing their TA viewing date in all different combinations. Make sure that not only can the date be changed to any valid date, but also that the listing of the gradeable on the gradeables page is valid. For example, if open submission date is in the past, and TA viewing date in the future, then the assignment should show under OPEN, not under FUTURE, and not under BETA. Secondly, On the gradeable page, make sure all the buttons are visible and function properly. For example, if the assignment shows as OPEN, all appropriate buttons should be visible and should function like normal. Finally, go to a gradeable labeled FUTURE and a gradeable labeled BETA and try to copy the link. Then log out and log in as a student and try to paste the link. You should NOT be able to access the gradeable. If you can, please report it immediately. Note: While testing, the only dates that should have full free access to be changed without causing any constraint issues currently are the grade inquiry dates and the ta view date. The rest of the dates still have constraints on them. ### Automated Testing & Documentation <!-- Is this feature sufficiently tested by unit tests and end-to-end tests? If this PR does not add/update the necessary automated tests, write a new GitHub issue and link it below. Is this feature sufficiently documented on submitty.org? Link related PRs or new GitHub issue to update documentation. --> This feature is sufficiently tested by unit tests and cypress tests. Cypress tests have been added to ensure that the gradables are visible to the proper users when the TA beta testing date is changed along with the other dates, especially in ways that would previously violate a constraint. I have also added additional cypress tests for lab and other gradeables to ensure that the TA view date along with the other dates are working properly without the constraint. These tests also mainly consist of moving the dates around and checking to ensure the gradeable is viewed by the proper access levels. ### Other information <!-- Is this a breaking change? Does this PR include migrations to update existing installations? Are there security concerns with this PR? --> This is not a breakage change. However, this PR DID add a create a new migration for the course database. It removed any DB constraints from the gradeable table relating to g_ta_view_start_date. Link hijacking might be a security concern with this PR and should be tested. Ensure that ONLY instructors have access to FUTURE gradeables and instructors and TAs/Mentors have access to BETA gradeables.
### CAUTION: Since this PR removes a date constraint, if it is installed on a production system and then needs to be reverted after a moderate period of time where gradeables might have been created/edited and taking advantage of the more flexible date settings, rolling back / reverting the change might fail. ### Why is this Change Important & Necessary? Partially addresses Submitty#12349 This PR specifically removes all constraints for the TA beta testing date for gradeables. This is important and necessary because instructors felt that the current UI for setting dates on gradeables was confusing, and they want to have more freedom over what dates they set for gradeables, even if they are invalid, since a lot of those dates may not matter for the specific class or assignment. ### What is the New Behavior? Now, the TA beta testing date has no constraints, it can be set to whatever date regardless of other dates. When one of the lower dates is set to before the present day and the TA beta testing date is set to the future, the lower date wins when assigning a gradeable status on the gradeables page. A date with actual constraints: <img width="2960" height="1755" alt="Constraint visible" src="https://github.com/user-attachments/assets/17cf85c4-cc63-4171-bee1-b1f7b37a0875" /> Previously, the TA beta testing date also had a constraint (where its box would turn red and changes would fail to save) if it was after any of the dates below it (For example: open submission date). Now, when the TA beta testing date is ahead of one of the dates below it, such as the open submission date (Only one that really matters now since all the other dates have constraints), it will display a message like the one shown here: <img width="3000" height="1891" alt="image" src="https://github.com/user-attachments/assets/b1c80060-bdc1-4e82-81a9-ce954a8aadfd" /> Additionally, when the TA beta testing date is in the future, and the open submission date is in the past, the open submission date wins and the assignment shows as open. This is also true for all dates below the TA viewing date. ### What steps should a reviewer take to reproduce or test the bug or new feature? Log in as instructor. Go to the sample course, try editing all gradeables and try changing their TA viewing date in all different combinations. Make sure that not only can the date be changed to any valid date, but also that the listing of the gradeable on the gradeables page is valid. For example, if open submission date is in the past, and TA viewing date in the future, then the assignment should show under OPEN, not under FUTURE, and not under BETA. Secondly, On the gradeable page, make sure all the buttons are visible and function properly. For example, if the assignment shows as OPEN, all appropriate buttons should be visible and should function like normal. Finally, go to a gradeable labeled FUTURE and a gradeable labeled BETA and try to copy the link. Then log out and log in as a student and try to paste the link. You should NOT be able to access the gradeable. If you can, please report it immediately. Note: While testing, the only dates that should have full free access to be changed without causing any constraint issues currently are the grade inquiry dates and the ta view date. The rest of the dates still have constraints on them. ### Automated Testing & Documentation <!-- Is this feature sufficiently tested by unit tests and end-to-end tests? If this PR does not add/update the necessary automated tests, write a new GitHub issue and link it below. Is this feature sufficiently documented on submitty.org? Link related PRs or new GitHub issue to update documentation. --> This feature is sufficiently tested by unit tests and cypress tests. Cypress tests have been added to ensure that the gradables are visible to the proper users when the TA beta testing date is changed along with the other dates, especially in ways that would previously violate a constraint. I have also added additional cypress tests for lab and other gradeables to ensure that the TA view date along with the other dates are working properly without the constraint. These tests also mainly consist of moving the dates around and checking to ensure the gradeable is viewed by the proper access levels. ### Other information <!-- Is this a breaking change? Does this PR include migrations to update existing installations? Are there security concerns with this PR? --> This is not a breakage change. However, this PR DID add a create a new migration for the course database. It removed any DB constraints from the gradeable table relating to g_ta_view_start_date. Link hijacking might be a security concern with this PR and should be tested. Ensure that ONLY instructors have access to FUTURE gradeables and instructors and TAs/Mentors have access to BETA gradeables.
9 participants
CAUTION: Since this PR removes a date constraint, if it is installed on a production system and then needs to be reverted after a moderate period of time where gradeables might have been created/edited and taking advantage of the more flexible date settings, rolling back / reverting the change might fail.
Why is this Change Important & Necessary?
Partially addresses #12349
This PR specifically removes all constraints for the TA beta testing date for gradeables. This is important and necessary because instructors felt that the current UI for setting dates on gradeables was confusing, and they want to have more freedom over what dates they set for gradeables, even if they are invalid, since a lot of those dates may not matter for the specific class or assignment.
What is the New Behavior?
Now, the TA beta testing date has no constraints, it can be set to whatever date regardless of other dates. When one of the lower dates is set to before the present day and the TA beta testing date is set to the future, the lower date wins when assigning a gradeable status on the gradeables page.
A date with actual constraints:
Previously, the TA beta testing date also had a constraint (where its box would turn red and changes would fail to save) if it was after any of the dates below it (For example: open submission date).
Now, when the TA beta testing date is ahead of one of the dates below it, such as the open submission date (Only one that really matters now since all the other dates have constraints), it will display a message like the one shown here:
Additionally, when the TA beta testing date is in the future, and the open submission date is in the past, the open submission date wins and the assignment shows as open. This is also true for all dates below the TA viewing date.
What steps should a reviewer take to reproduce or test the bug or new feature?
Log in as instructor. Go to the sample course, try editing all gradeables and try changing their TA viewing date in all different combinations. Make sure that not only can the date be changed to any valid date, but also that the listing of the gradeable on the gradeables page is valid. For example, if open submission date is in the past, and TA viewing date in the future, then the assignment should show under OPEN, not under FUTURE, and not under BETA.
Secondly, On the gradeable page, make sure all the buttons are visible and function properly. For example, if the assignment shows as OPEN, all appropriate buttons should be visible and should function like normal.
Finally, go to a gradeable labeled FUTURE and a gradeable labeled BETA and try to copy the link. Then log out and log in as a student and try to paste the link. You should NOT be able to access the gradeable. If you can, please report it immediately.
Note: While testing, the only dates that should have full free access to be changed without causing any constraint issues currently are the grade inquiry dates and the ta view date. The rest of the dates still have constraints on them.
Automated Testing & Documentation
This feature is sufficiently tested by unit tests and cypress tests. Cypress tests have been added to ensure that the gradables are visible to the proper users when the TA beta testing date is changed along with the other dates, especially in ways that would previously violate a constraint. I have also added additional cypress tests for lab and other gradeables to ensure that the TA view date along with the other dates are working properly without the constraint. These tests also mainly consist of moving the dates around and checking to ensure the gradeable is viewed by the proper access levels.
Other information
This is not a breakage change. However, this PR DID add a create a new migration for the course database. It removed any DB constraints from the gradeable table relating to g_ta_view_start_date.
Link hijacking might be a security concern with this PR and should be tested. Ensure that ONLY instructors have access to FUTURE gradeables and instructors and TAs/Mentors have access to BETA gradeables.