Closed bzamora020 closed 3 years ago
Nice PR! Your PR can be merged once you get an approving review from a staff member and a member of your team.
Keep in mind, code reviews only count if you use the "Approve" or "Request changes" option in GitHub. You can find this under the "Files changed" tab under the "Review changes" button.
Generated by :no_entry_sign: dangerJS against 8062c14f38b15a4fa4ee5731f683e9ae46e0a7b6
Merging #281 (8062c14) into main (5ebeaf5) will decrease coverage by
0.12%
. The diff coverage is98.22%
.
@@ Coverage Diff @@
## main #281 +/- ##
============================================
- Coverage 99.75% 99.63% -0.13%
- Complexity 406 439 +33
============================================
Files 99 101 +2
Lines 2078 2205 +127
Branches 97 104 +7
============================================
+ Hits 2073 2197 +124
- Misses 5 8 +3
Impacted Files | Coverage Δ | |
---|---|---|
...a/edu/ucsb/ucsbcslas/entities/TutorAssignment.java | 100.00% <ø> (ø) |
|
...src/main/pages/TutorAssignment/TutorAssignments.js | 84.00% <75.00%> (-10.45%) |
:arrow_down: |
...onents/TutorAssignment/TutorAssignmentCSVButton.js | 100.00% <100.00%> (ø) |
|
javascript/src/main/pages/Tutor/Tutor.js | 88.46% <100.00%> (ø) |
|
...services/TutorAssignment/TutorAssignmentService.js | 100.00% <100.00%> (ø) |
|
...sbcslas/controllers/TutorAssignmentController.java | 100.00% <100.00%> (ø) |
|
...du/ucsb/ucsbcslas/controllers/TutorController.java | 100.00% <100.00%> (ø) |
|
...du/ucsb/ucsbcslas/models/TutorAssignmentModel.java | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 5ebeaf5...8062c14. Read the comment docs.
Overall looks good, I have some small requested changes.
Secondly, would it be possible to display the "Upload CSV " Button when no Tutor Assignments exist, essentially whenever the "New Tutor Assignment" Button is displayed?
I added the showing the upload CSV part to it, deleted the comments, removed the not used imports, made it check both on front and backend that the user is an authorized user to make an upload CSV request.
When I introduced the way to check for an authorized user to make an upload csv request, the coverage of a line that was previously covered is not covered. Is it possible to make a PR after to fix that?
The bug appears to be from white space around entity field values, when there should not be whitespace. The fix is to trim white space when setting each value in TutorAssignmentModel.java
, as the parse does not use the parametrized constructor rather uses the set methods. Thus the parametrized constructor can also be removed from TutorAssignmentModel.java
.
In this PR, there has been added the functionality to upload a CSV to add tutor assignments on a mass scale instead of adding tutor assignments one by one. If there has not been created a previous Tutor or Course, it will create them. If they have been created, it won't modify them. The same case with the tutor assignment, this is to avoid duplicates.
NOTE: There is 3 lines not covered in the test coverage, due to this happened to a previous PR and it is pretty tedious to make the test for those, it will be necessary to create an issue to fix that in a PR. Once this PR is merged