digitalfabrik / integreat-cms

Simplified content management back end for the Integreat App - a multilingual information platform for newcomers
https://digitalfabrik.github.io/integreat-cms/
Apache License 2.0
56 stars 35 forks source link

Models not validated properly on XLIFF import #454

Closed timobrembeck closed 3 years ago

timobrembeck commented 4 years ago

Describe the Bug

At the moment, all model validation is placed in the forms, because this allows for better error handling and more informative error messages in forms. However, when using different methods to create content (e.g. the xliff import), these validation methods don't work anymore. Therefore, we need also validation on the model-level.

Steps to Reproduce

  1. Go to page tree
  2. Import XLIFF-file with e.g. empty page titles
  3. Confirm

Expected Behavior

If the imported data is not valid, it should not be saved to the database.

Actual Behavior

The invalid data is imported.

svenseeberg commented 4 years ago

I'm not sure if validating on the model level is the best solution. We could say that we either use forms OR do some additional validation during IO. The question is, how we make sure that this validation is complete? We could have some sort of helper class that validates a dict (adhering to a set of rules, i.e. keys need to be the same as model properties) against model properties?

timobrembeck commented 4 years ago

I'm not sure if validating on the model level is the best solution. We could say that we either use forms OR do some additional validation during IO. The question is, how we make sure that this validation is complete? We could have some sort of helper class that validates a dict (adhering to a set of rules, i.e. keys need to be the same as model properties) against model properties?

I'm not sure if I understood this correctly. We definitely wouldn't need some kind of custom helper class to validate a dict, because Django already has very powerful validation mechanisms on the model level (similar to form validation, see docu)...

svenseeberg commented 4 years ago

I see your point - I supposed that validation is being done on the form layer. However, I would still insist that we need different levels of validation. For example manually editing pages vs XLIFF imports. While I would ask a user to enter a title while filling in the page edit form, I would allow XLIFF imports with empty titles. The problem with XLIFF files is that most users are not able to edit them, and sending them back to translators is not always an option. Therefore it should be possible to import and fix some stuff later in the back end.

timobrembeck commented 4 years ago

Ah ok, interesting. Never thought of this workflow. Maybe they should be imported as status "in-review" (or we create a new status "invalid") to make it clear that they need a manual review before they can be published...