Fixes #259 Reflect file deletion by removing the whole entry

[arkirchner]

Remove the hole file element form the exercise form when a file is deleted.

This issue is described in #259

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.31%. Comparing base (0a3119c) to head (ccb652e).
Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2922   +/-   ##
=======================================
  Coverage   69.31%   69.31%           
=======================================
  Files         211      211           
  Lines        6755     6755           
=======================================
  Hits         4682     4682           
  Misses       2073     2073           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[MrSerth]

Thanks for your first PR and congratulations on fixing your first bug! I just checked the changes locally and think they are working as intended 👍

As you can imagine, I have a few comments and thoughts to share:

• Have you checked whether we can change the immediate DELETE to a regular deletion of the respective file during the PATCH? Maybe by setting _destroy and using accepts_nested_attributes_for :files, allow_destroy: true? This way, we could remove the JavaScript alert and do any updates combined with the file deletion in one transaction. I would like to see this as a follow-up improvement. What do you think?
• I know there is no spec for editing an exercise as a teacher. Wouldn't this be a good opportunity to start a (system) spec?
• Your commit message contains some typos, such as hole –> whole, or the first form –> from. Also, I've historically put the issue not as prominent in the message. We have different issue naming schemes for the projects. While it is common for openHPI / Xikolo to have a ticket as XI-1234 (similar to your style), in the other projects with local issues, we usually use #1234. It's a matter of taste, but I tend to include the ticket number as the last line (Fixes #1234, Closes #1234), allowing GitHub to link it. Of course, I see benefits in making the issue more prominent 🙂 If you haven't done so, I recommend checking out some of our internal guides, where you'll find more on this topic: https://confluence.hpi.de/x/xYGbB. In many GitHub projects, we derived from some parts, such as the branch naming convention, making your choice totally fine. As a last remark, I would suggest adding some JavaScript/form/exercise reference to the first line (if space permits), to distinguish from the other aspects where we handle files (in submissions, as downloadable artefacts, for executions).

[arkirchner]

Thank you very much for your comment.

I know there is no spec for editing an exercise as a teacher. Wouldn't this be a good opportunity to start a (system) spec?

I wasn't sure about the policy of adding system tests. I am wary of adding too many system tests because of the brittleness UI tests introduce. Deleting the file from the form might be worth testing 🤔

What we should test is:
Create an exercise and files
Edit an exercise and edit and delete a file.

What do you think?

_destroy and using accepts_nested_attributes_for :files, allow_destroy: true

Maybe the form could be improved in this manner. I am not a big fan of the accepts_nested_attributes_for because I think it leaks view logic into the model layer. Maybe it would be better to create a form object to deal with that if we want to do both at the same time.

To improve this form, my thought was to separate creating exercises and files. This would make the UI simpler and the implementation of each part smaller.

At this point, I don't know who is using this form and what he main challenges are. So I refrained from making functional changes until I know a little bit more.

[MrSerth]

I know there is no spec for editing an exercise as a teacher. Wouldn't this be a good opportunity to start a (system) spec?

I wasn't sure about the policy of adding system tests. I am wary of adding too many system tests because of the brittleness UI tests introduce. Deleting the file from the form might be worth testing 🤔

What we should test is:
Create an exercise and files
Edit an exercise and edit and delete a file.

What do you think?

Sounds good! Of course, UI tests come at a cost, particularly the tight relationship to the UI and the slow execution. Hence, I agree to use them mainly for important parts. I would consider the exercise creation and update to be such a part and think it can help to find/prevent errors.

_destroy and using accepts_nested_attributes_for :files, allow_destroy: true

Maybe the form could be improved in this manner. I am not a big fan of the accepts_nested_attributes_for because I think it leaks view logic into the model layer. Maybe it would be better to create a form object to deal with that if we want to do both at the same time.

I definitely see that and think there are better options. However, we currently use accepts_nested_attributes_for for this part already, so the change would only be to amend allow_destroy: true. That's still not ideal, for sure, but it would allow us to remove the AJAX request, drop the DELETE /files route (along with the policy), and ensure transactional updates. I'd say that's worth the effort and allows us to improve the form later on.

To improve this form, my thought was to separate creating exercises and files. This would make the UI simpler and the implementation of each part smaller.

I am very open to new ideas and improvements. How would the workflow look then? I am not sure a total separation (and linking) makes sense, since a file can (currently) only exist as part of a submission or exercise.

At this point, I don't know who is using this form and what he main challenges are. So I refrained from making functional changes until I know a little bit more.

The form is only used by educators; changing the files is (in my opinion) one of the most used tasks when dealing with exercises after their initial creation. Despite the room for improvement, the existing appearance was mostly fine for many educators. Hence, I would go with incremental improvements here first, and gain a better overview of the system and (technical) challenges.