[openstreetmap/openstreetmap-website] Added a validation to the Redaction model for the title. (#2034)

Andy Allan notifications at github.com
Wed Sep 16 09:50:07 UTC 2020


First of all, I'm sorry that it has taken so long to respond here.

I've reviewed this PR a couple of times over the last few months, and mulled over the options, and there's a number of issues with it, including:

* Tests are added for both `title` and `description`, but the migration only covers the `title`.
* The model tests can be simplified by using e.g. `redaction.valid?` instead of attempting to save and then asserting against the errors.
* A lot of comments are added to existing tests, but they mostly just restate exactly what the code already says.
* Due to changes in the meantime, this PR now conflicts with master.

But the big thing is that we've introduced the 'strong_migrations' gem, which flags up this migration as potentially locking the database while it runs. So the migration will have to be substantially re-written to deal with this.

My recommendation for reworking this PR is to split it into two parts - one for the additional model tests (which only test the model validations anyway, not the database constraints) and a separate one for the migration, rewritten to be compatible with `strong_migrations`.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/2034#issuecomment-693298705
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20200916/e4b61007/attachment.htm>


More information about the rails-dev mailing list