codeforamerica / MuniciPal

:speech_balloon: Consulting city-dwellers about legislation near them.
22 stars 14 forks source link

handle missing social accounts for people #59

Closed techieshark closed 10 years ago

techieshark commented 10 years ago
tbuckl commented 10 years ago

this works for facebook. but not for twitter. for city manager, shows MesaDistrict6 account?

tbuckl commented 10 years ago

ok i can merge this because it seems to work for facebook. and then the most recent PR for social media (#63) seems to work for twitter. but maybe it makes sense to close both of the social PR's, merge them, and submit them together?

techieshark commented 10 years ago

let me double check this before you do anything else on it. thanks.

techieshark commented 10 years ago

It is maybe a bit tricky to verify this, but the idea is that if social is set to [] in the council-info/people.js, then this.socialDetails('twitter') on person.js:65 (https://github.com/codeforamerica/MuniciPal/blob/c40d3664e55b949c65d08544970c54d2f6228c7d/app/assets/javascripts/person.js#L65) will return null / false and line 68 will be triggered which renders the 'noTwitterTemplate'.

The reason you don't see it right now is because the Manager actually has a social entry in the council-info repo.

If you wanted to try it out, you could either remove the twitter object from this line: https://github.com/techieshark/council-info/blob/gh-pages/people.js#L186

or you could set a breakpoint at person.js:65 (before if(this.socialDetails('twitter')) is called, and in the console run this.person.social = [] to fake having no twitter values and then run it from there and you'll see the correct output under the twitter tab.

tbuckl commented 10 years ago

peter, is this up for review still? seems like production on heroku has this fix?

techieshark commented 10 years ago

Check out the manager page on production now. This commit makes it so his social account is missing and you can see the result on production and compare to the result proposed in this branch.

tbuckl commented 10 years ago

ah ok, thanks. so this works for the city manager but then i get this for the mayor: image

techieshark commented 10 years ago

@buckleytom that's fine, the problem you mention there is a separate issue that was fixed in master already by this commit: https://github.com/codeforamerica/MuniciPal/commit/6aa9f4fc43aaab664cb3d7f35daa6d2efed5f0f1

If you want to see this working locally, try this:

git checkout master
git pull
git checkout -b master-merge-test
git merge  frontend/handle-missing-social-accounts
rails s

That will have the fix for the missing social media account from this commit, plus the fix for the missing widget already in master that I mentioned, and you should see the Mayor's twitter widget as well as the message on the City Manager's page about his missing account.