search

mysociety / yournextrepresentative

A website for crowd-sourcing structured election candidate data
https://candidates.democracyclub.org.uk/
GNU Affero General Public License v3.0
56 stars 21 forks source link

Log the rejection reason for photo moderation operations #905

Closed struan closed 8 years ago

struan commented 8 years ago

This adds a note field to the LoggedActions model and uses that to store the rejection reason when uploaded photos are rejected during moderation.

It also adds a migration to update the help text of the complex fields because there was a mismatch there.

Fixes #896

wfdd commented 8 years ago

Looks like another migration got caught up in this?

struan commented 8 years ago

I deliberately added that migration in because it looks like it was missed earlier and it without it I was having to edit the makemigrations output.

wfdd commented 8 years ago

Oh right, sorry.

mhl commented 8 years ago

It would be good to have the missing migration in a separate commit, I think (with the commit message referencing the commit it should have been included in). Otherwise this looks good to me :+1:

mhl commented 8 years ago

The test failure would be fixed by 3e6b734c79f3017a5b4b0c1bc102fbb8adc3dea4, I think

mhl commented 8 years ago

One other thing about this - I think should be simple to update one of the existing tests to check that the reason is recorded in the LoggedAction in the case of rejecting a photo, so it would be good to do that too.

mhl commented 8 years ago

Sorry, @struan - one last comment, I promise! I think the commit message for https://github.com/mysociety/yournextrepresentative/pull/905/commits/e3787996c05cea4acf0011b2a11d9e72c79827bd should indicate that it's fixing a mismatch between the model and migrations. (The commit that added the migration to create the table didn't match the model in candidates/models/fields.py in the same commit, I guess because the models were fixed-up without regenerating the migration.)

The way the commit message is phrased at the moment I'd expect the commit to have the change both in the models and migrations, and really it's only making sure the migrations are back in sync with the model.