search

diaspora / diaspora-project-site

Code for the Diaspora project site.
https://diasporafoundation.org/
Other
47 stars 41 forks source link

Fix mailing_list_link in getting_started #127

Closed goobertron closed 6 years ago

goobertron commented 6 years ago

As a temporary fix, I've reverted the link name from discourse_link to mailing_list_link. While it's not an ideal name for a link to Discourse, at least it will fix the broken translations for now.

As before, I've not been able to test this because my instance is broken. But I felt obliged to try to fix it as I broke it.

@SuperTux88 @denschub would be grateful if one of you would check this out and push it when you get a chance.

denschub commented 6 years ago

That won't cut it, I fear, as some languages already have changed it. It might be a workaround to just define mailing_list_link in addition to discourse_link, but leave the translations at discourse_link. That way, if translations still use the old variable, they'd be fine.

This assumes rails does not complain if there is an unused parameter. I cannot verify that assumption right now, @goobertron, can you give that a try?

denschub commented 6 years ago

As before, I've not been able to test this because my instance is broken.

nevermind. I should really read things. Please just change your patch to define both variables, but keep the translation unchanged. I'll run tests in a bit.

Edit: It works. So just add a

          mailing_list_link: link_to(t('.discourse_link_text'), 'https://discourse.diasporafoundation.org/'),

and we'll be good.

goobertron commented 6 years ago

OK, hopefully that will allow all translations to work: I've simply reinstated the mailing_list_link_text string to en.yml.

goobertron commented 6 years ago

Oh, I've just seen your edit. OK: given me a second.

goobertron commented 6 years ago

@denschub done! Thanks for the advice.

I must get an idiot's guide tutorial from you on how to update things such as this so that translations won't break in future!

denschub commented 6 years ago

I took the liberty to force-push over your PR branch, as this is what I wanted: https://github.com/diaspora/diaspora-project-site/pull/127/commits/6a66c2579802745ec1ca1ef5c25b5b7a3a3e8980

That way, translations using the old %{mailing_list_link} will still continue to work, but simply link to discourse. That's not nice, but a okay'ish temporary workaround for now. Unfortunately, I wasn't able to describe to you what I want in a hurry, and I need to get this thing merged and deployed right now.

I must get an idiot's guide tutorial from you on how to update things such as this so that translations won't break in future!

Actually, it wasn't really your fault. On merging, we should have invalidated all translations using the old parameter on WebTranslateIt - which we missed. Something to learn for next time.

goobertron commented 6 years ago

I took the liberty to force-push over your PR branch, as this is what I wanted: 6a66c25

Thanks – I just remembered to squash the commits and have done it, but too late.

Actually, it wasn't really your fault. On merging, we should have invalidated all translations using the old parameter on WebTranslateIt - which we missed. Something to learn for next time.

That's good to know. If I ever feel I need to change a parameter name in future, I'll try to remember to flag this in the PR comments.

denschub commented 6 years ago

Merged, and deployed. Thanks for taking care of it, @goobertron.

Just one note, to be sure:

But I felt obliged to try to fix it as I broke it.

I appreciate you taking responsibilities for your work - but you don't have to. If you want, that's fine, but please don't feel forced. Like, ever. :)

goobertron commented 6 years ago

Thanks, but I felt morally obliged by my own embarrassment! Not forced by anyone else. Just wanted to clean up my own mess where I could! :-)

SuperTux88 commented 6 years ago

But I felt obliged to try to fix it as I broke it.

Oh, I broke it at least as much as you, because I reviewed it and didn't notice it.

Actually, it wasn't really your fault. On merging, we should have invalidated all translations using the old parameter on WebTranslateIt - which we missed.

I wouldn't do that, because there is no need to invalidate a valid translation only because the parameter changed. I think adding a temporary param and then changing it in all translations is a better solution.

Anyway, thank you to for fixing it :cookie: :cookie: