mysociety / alaveteli

Provide a Freedom of Information request system for your jurisdiction
https://alaveteli.org
Other
386 stars 196 forks source link

If a translated public body name is added for a locale after requests have been made to the body, requests to the body are not appearing on the body page in the new locale #768

Closed crowbot closed 11 years ago

crowbot commented 11 years ago

See, for example, http://www.whatdotheyknow.com/cy/body/adran_yr_amgylchedd_bwyd_a_materion_gwledig

crowbot commented 11 years ago

Hi,

First of all, thanks for tackling this, and I'm sorry for the delay in looking at it - having had a proper look at it now, sadly I think it may all be a bit of a wild goose chase. I should first note a couple of extra things. When @RichardTaylor first reported this issue with reference to the authority Defra in WDTK, it was seven hours after he had added a Welsh name for the body, and no requests were appearing on the Welsh version of the page, referenced above. However, now I see that the page is, in fact, showing the same number of requests in the Welsh and English versions.

Looking at the code, I see the search for the requests uses the criteria " requested_from:#{@public_body.url_name}". When a new translated name for the public body has been added, a new url_name for the locale is generated to go with it (https://github.com/mysociety/alaveteli/blob/develop/app/models/public_body.rb#L251). The problem is that searches with that new url_name in the new locale weren't producing the expected requests. I can see that initially that makes sense - as no request events have been indexed as having be requested from the new locale url_name. However, the public body model already has some code that ought to handle this - https://github.com/mysociety/alaveteli/blob/develop/app/models/public_body.rb#L233 - if the name is changed, it should get all the associated request events to be reindexed, and https://github.com/mysociety/alaveteli/blob/develop/app/models/info_request_event.rb#L121 should ensure that the new url_name is included. Request events should be indexed with requested_from fields for every translated name for the public body.

So the expected behaviour is that the controller code as it is before this pull request ought to work after the reindexing delay. But the bodies weren't showing up within a reasonable amount of time. Now, some weeks later they're there, indicating that once those request events are reindexed with the new name, everything works OK, but perhaps the reindexing is not getting triggered immediately when it should be. If reindexing isn't being triggered properly, then things like advanced search queries using "requested_from:[some localised url_name]" will fail as well.

Looking at the tests added in this pull request, I think one problem is that they don't quite represent the situation as it was. They don't use the new Spanish url_name in the spanish tests. Somewhat confusingly, because public_body takes short_name in preference to name when generating a new url_name, setting the name only in the spanish locale seems to pull through the short_name from the default locale, meaning the new spanish shortname would be 'sensewalk'. The test 'should show the same requests in all locales after adding translation (es)' fails before any changes to the controller code not because a different number of results is being returned, but because it's trying to do a redirect to 'sensewalk' from 'sensible_walks' before it ever gets to the search code.

Having changed the tests a bit to represent better what I think the real life situation was https://github.com/mysociety/alaveteli/blob/feature/issue_768_tests/spec/controllers/public_body_controller_spec.rb#L61, sadly they pass, so I'm a bit at a loss as to whether there's a genuine problem here :(. Perhaps there was some confusion over which translated name was added when (although the link in the original bug report was to defra, the text of the bug report referenced a different body, dwp) our reindexing was just delayed for some reason.

mlandauer commented 11 years ago

I think I need a clear head to go through what you're saying look at this in detail. So, I think that's a task for tomorrow morning. ;-)

crowbot commented 11 years ago

Totally makes sense - sorry for the long account :)

crowbot commented 11 years ago

I'm going to close this. @mlandauer, @RichardTaylor please feel free to re-open it if you think there's clearly a bug to fix.