refactor: Files feedback_message without NULL state

[arkirchner]

Column type string should not have a null state to avoid logical
distinction between NULL and '' case.

User defined tests are created without feedback_message. Removing
the NULL case allows the new record to render the empty feedback_message.

An edge case is resolved when a test for assessment with a feedback_message
is transformed to a user defined test. The feedback_message is
overwritten, and no validation error for invisible fields is displayed.

Resolves #3014

[arkirchner]

I want to make clear that the create does not create a record here. Therefore, I use assignment and call validate.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.09%. Comparing base (ea29353) to head (456128c).
⚠️ Report is 21 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3046   +/-   ##
=======================================
  Coverage   70.08%   70.09%           
=======================================
  Files         215      215           
  Lines        6850     6851    +1     
=======================================
+ Hits         4801     4802    +1     
  Misses       2049     2049           

☔ 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.

[kkoehn]

would adding a spec to test for the bug in #3014 be an option?

[MrSerth]

I am not happy to set a blank string as a solution here. This adds further custom code (like a custom validation rather than using a built-in validation), makes analysis of the code files more difficult, and potentially increases data storage requirements.

Which other solutions to the problem did you consider? Why did you decide that way?

[arkirchner]

If a column can be NULL or "", we have two different ways to represent “no content.” In practice, these two states often require the same handling, so it’s easier to have just one. It is frequently fine to run methods on an empty string like ''.underscore. Data retrieval becomes simpler because we don't need to check for both states WHERE name IS NULL OR name = ''.

Thoughtbot includes this in their excellent developing guides. There is also a very famous blog post on the NULL state that explains the ideas on a higher level.

These blog posts on the matter are also superb:

• Manuel Ramallo - I don't use nil
• Joshua Paling - Nulls In Your Database

A good Rails application as an example of this implementation is Mastodon.
https://github.com/mastodon/mastodon/blob/main/db/schema.rb

While this is not a standard that is adopted by everyone I am a big fan. This data normalization avoids conditional logic and makes the code more readable.

[MrSerth]

Thanks for detailing your thoughts.

If a column can be NULL or "", we have two different ways to represent “no content.”

I disagree. Only NULL represents "no content", whereas '' represents "empty content". From a database design, that is a fundamentally difference.

In practice, these two states often require the same handling, so it’s easier to have just one.

This is a statement about a view, not about persistency. I see a logical differentiation between both, and an arguably "easier" access method should not influence the data modeling.

[various blog posts]

After reading these blog posts, I think only two of them actually address NULL values in the database, while the others are generally dealing with nil in Ruby (which might be different from the data modeling).

While this is not a standard that is adopted by everyone I am a big fan. This data normalization avoids conditional logic and makes the code more readable.

After careful considerations and some discussions with experienced engineers, my mind hasn't changed: Data modeling in the database should be based on the business logic, and not follow an easier access pattern for the views. Also, I want to note that we could strive for two kinds of values here, too: a present, not empty string for teacher-defined assessments and NULL otherwise. Therewith, we would only have two cases to deal with.

[arkirchner]

Data modeling in the database should be based on the business logic

What is the business case that requires differentiation between NULL and "". After carefully checking the code, I was unable to find one.

[arkirchner]

Since there are fundamental differences between our views of what good software design is I will close this PR. I will check for the file_role to avoid this error.

I have avoided this solution because it adds further variants to code that could have been avoided by data normalization.

[MrSerth]

Data modeling in the database should be based on the business logic

What is the business case that requires differentiation between NULL and "". After carefully checking the code, I was unable to find one.

So far, we don't need/require '' at all (and, ideally, it shouldn't be in place in the database), so that this question (about a differentiation between both) does not arise. The business logic I referred to is the chosen design with different file types being stored in the same table (similar to single-table inheritance), with a missing feedback_message for those files were none is allowed/expected (a main_file should not have any feedback message, because it has no semantic meaning).

Since there are fundamental differences between our views of what good software design is I will close this PR. I will check for the file_role to avoid this error.

Thank you for considering that; I'll expect a rather simple change to avoid the bug in question. Also, I am open to hearing other opinions, so in case you want to ask the team to have another look and share their perspectives, you are more than welcome to.

I have avoided this solution because it adds further variants to code that could have been avoided by data normalization.

I am much in favor of data normalization. What about adding a migration to convert all empty strings '' to NULL values and ensure no empty string is accepted any longer? This also achieves data normalization and reduces the variants to two (NULL or non-empty string). At the same time, this approach would follow the semantic meaning.