Added: Add server-side playlist validation to rules files

[drikusroor]

This pull request adds playlist validation to the rules files in order to ensure that the playlists used with each rule file meet the specific requirements. The validation is implemented by updating the rule base class with a validate method. This validation method is then used in the experiment form's (Django built-in clean_{property_name}) clean_playlists method and adds errors if one or more experiment playlists are invalid based on the experiment's rules.

No longer dynamic client-side

This pull request adds playlist validation to the rules files in order to ensure that the playlists used with each rule file meet the specific requirements. The validation is implemented by updating the rule base class with a validate function. Additionally, an API endpoint is added for validating the experiment playlist based on the rules. The experiment form in the admin interface is updated to use the validation endpoint and display warnings and errors for playlist incompatibility. The pull request also includes test cases for playlist validation.

It looks like this:

Screen.Recording.2024-05-03.at.15.06.27.mov

Resolves #978

[BeritJanssen]

Can we override the field's clean method to achieve this? Cf. Django documentation. This does seem like a situation where Django could handle the logic, but I'm not sure if it gets more involved because Playlist is a ManyToManyField.

[albertas-jn]

Although I find it OK as it is, indeed, using built-in django form validation might be a simpler solution.

[drikusroor]

Can we override the field's clean method to achieve this? Cf. Django documentation. This does seem like a situation where Django could handle the logic, but I'm not sure if it gets more involved because Playlist is a ManyToManyField.

Although I find it OK as it is, indeed, using built-in django form validation might be a simpler solution.

I explicitly did not use Django's built-in form validation because I wanted to add dynamic validation (which happens before you submit the form already). What you y'all think: Would it be better to rewrite it to use Django's built-in form validation and risking it not to be a dynamic validation, or should we keep it like this?

[BeritJanssen]

My two cents would be that it's not too big a deal for the user if they clicked "save" and then get feedback about the playlist. It's a different story with things like uploading sections, where we'd have to repeat a rather involved process before fixing the problem, but here it's a checkmark that would need to be changed, so my preference would be Django built-in over custom Javascript, which is hard to unit test, and therefore hard to maintain.

[drikusroor]

My two cents would be that it's not too big a deal for the user if they clicked "save" and then get feedback about the playlist. It's a different story with things like uploading sections, where we'd have to repeat a rather involved process before fixing the problem, but here it's a checkmark that would need to be changed, so my preference would be Django built-in over custom Javascript, which is hard to unit test, and therefore hard to maintain.

I've removed the client-side (server rules based) validation and moved it to server-side validation using Django's built-in clean_{property_name} method.

However, I've kept the API endpoint as it allows us to call it in E2E tests to validate the playlists. Do you think that it's a good idea or should I also remove the validate_playlist/{rules_id} endpoint?

[BeritJanssen]

Nice! No, I think it's fine to keep the validate_playlist endpoint in place. If it turns out we don't use it (ideally, validation ensures that no faulty data is in the database, so we wouldn't need an e2e test / might use an e2e test only to catch and fix pre-existing problems), we can still remove it at a later stage.

[drikusroor]

Nice! No, I think it's fine to keep the validate_playlist endpoint in place. If it turns out we don't use it (ideally, validation ensures that no faulty data is in the database, so we wouldn't need an e2e test / might use an e2e test only to catch and fix pre-existing problems), we can still remove it at a later stage.

Yes, for example, I think the production server has an experiment with the GoldSmith rules (which don't exist anymore). Such cases can be caught using this endpoint.