Closed Melhaya closed 2 weeks ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 94.08%. Comparing base (
beb5df3
) to head (5762da4
). Report is 3 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@MrSerth Thanks for identifying the issue.
I have added a validation in the TaskFile
and adjusted the creation of one in the GptService
.
Currently a few tests are failing though. I am not sure how to tackle this. Of course due to the added validation in the TaskFile
, these errors are raised. I am just not sure how to fix them. Could you please advise?
Am I supposed to change all create(:task_file
) in the specs to include visible: 'no'
?
Update: I have instead created a default value for visible
set to 'no' when creating or updating a TaskFile
. Errors are now fixed. Not sure if this is a valid solution though.
Thanks
Since @kkoehn is an expert on the ProFormA standard and valid values, I am passing the review request over to him.
I would not fix it in the before_validation
, but in the factory for the tests.
I noticed that used_by_grader
is also required per ProFormA standard. It would make sense to implement a validation for that, too.
Currently, when creating a test-file through the UI, visible
has the default-value yes
(used_by_grader
is not set by default).
I think we could extend the scope of this PR to also add a validation for used_by_grader
, as well as the frontend modification to select a default value (should just be adding a checked: true
in the correct line in views/tasks/_file_config.html.slim
)
Concerning which default values to choose: There are no rules regarding that. I thought it makes sense, that a file added through the UI would be visible by default. I think it would make sense to extend that to your generated test files. By extension I would choose true
for used_by_grader
as default.
Hey @kkoehn, Thanks for your comment. what do you mean by "I would not fix it in the before_validation
, but in the factory for the tests." where is the factory for tests? Since I added the default value in GptServices, are you saying there is no need to do the before_validation
anymore at this point? But then when I remove the before_validation
line, many tests will fail as they are not created with a default visible
value
in GptService:
test_file = TaskFile.new(content: gpt_response, name: file_name, used_by_grader: true, visible: 'yes', xml_id: SecureRandom.uuid)
The default value for used_by_grader
and visible
are set correctly. What else should be implemented?
I compiled a small todo-list from my jabbering:
before_validation
-hookused_by_grader
used_by_grader
in mentioned view-templateThe factories are located in spec/factories/
. There you are look for task_files.rb
.
@kkoehn Thanks for the to-do list. it helped a lot.
The following commit addresses all of them I think. feel free to check it out and check your to-do list if the request is met :)
@kkoehn Thanks for the hint and information regarding boolean values :)
I have added the spec for the visible
value in the latest commit
@MrSerth Thanks for the quick review :)
For the migration, I am honestly not sure how to do that by myself as it is a confusing area for me, and would need assistance to do it. If you would also prefer to do it to get it done faster, that's fine with me :)
I added a migration, that should fix the existing TaskFiles Please take a look
Related to https://github.com/openHPI/codeharbor/issues/1576 raised by @MrSerth
Actions taken: