search

cyclestreets / cyclescape

Cyclescape - cycle campaign group toolkit
https://www.cyclescape.org/
MIT License
33 stars 15 forks source link

Tag editing failure #933

Closed mvl22 closed 4 years ago

mvl22 commented 4 years ago

/issues/2146 is giving a 409 response when editing the tags list on that page. I was trying to add the tag 'planning'.

mvl22 commented 4 years ago

Further example issues with the same problem:

1248, 2147, 425, 1864, 1040, 1998, 1278, 237

I can't see any particular pattern to why these might be affected.

Possibly Rollbar will give us a clue if reports come to that.

mvl22 commented 4 years ago

I think I've found why this is happening - it's because the issue title is too long, so that unrelated constraint is preventing the commit of the record saving.

Can the update be changed so that the tag update just does an update of that data and not try to amend the whole record?

nikolai-b commented 4 years ago

I don't think that is a good solution, it will fix this but then something else will come up where there is a complain that these issue can't be edited.

I suggest we relax the 80 char limit for issues that are already too long OR fix them. There are 71 issues where the title is too long:

> Issue.where("char_length(title) > 80").ids # Issue IDs that are currently too long
=> [2133, 2184, 1671, 1992, 1998, 2072, 2075, 375, 2078, 2125, 2146, 2147, 2170, 1656, 2178, 2176, 2180, 2182, 1651, 1779, 1782, 1906, 2185, 2197, 2198, 566, 2206, 2231, 2224, 2232, 163, 657, 213, 215, 233, 377, 378, 439, 425, 444, 496, 563, 621, 717, 724, 867, 984, 1008, 1022, 1040, 1195, 1239, 1234, 1245, 1250, 1278, 1285, 1318, 1377, 1443, 1504, 1507, 1458, 1637, 1692, 1742, 1864, 1869, 1892, 1917, 1955]
mvl22 commented 4 years ago

I think it's best for the titles to be fixed up as these arise, rather than exempt those from a constraint.

Is there any simple way that you can have the site return an alert (yes, rubbish I know but we don't need a massive UI for this) basically saying that the issue title needs to be shortened first. It's completely non-obvious why this fails. No worries though if this is not simple to do in Rails.

nikolai-b commented 4 years ago

Given this is causing problems I'd prefer to exempt them from the constraint and fix them as we go along. I should have spotted this this when I reduced the title length to 80. The exemption would be on the lines of: if the title is not being altered and the existing title is > 80 chars then ignore the char limit.

Adding an alert is possible but more work that exempting them from the constraint and it is possible there are other places in the code where we do something that causes the issue to be re-saved which will also unexpectedly fail.

mvl22 commented 4 years ago

OK. I don't have strong views on this - do fix as you feel is most sensible.