diaspora / diaspora-project-site

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

Mention EXIF in settings tutorial #128

Closed goobertron closed 5 years ago

goobertron commented 5 years ago

This adds mention of the EXIF settings in the 'Privacy' section of the user panel in Part 7 of the tutorial.

I've also deleted a duplicate line left over from #127 – not sure how that got in there. @denschub just alerting you to that in case I've done something stupid.

(I realise this is a very small PR, but one of the new members asked about the stripping of EXIF data to HQ so I wanted to add it as soon as possible.)

denschub commented 5 years ago

I've also deleted a duplicate line left over from #127 – not sure how that got in there. @denschub just alerting you to that in case I've done something stupid.

Hm. :) Apparently I didn't do a good enough job of explaining what happened, probably because I wanted the original PR to be resolved so we can fix a broken production site. So, let me try again.

Before your initial PR (#125), the locale string looked like the following:

join our %{mailing_list_link}

because you wanted to keep things named correctly, you decided against only changing the values, but also to rename the variable, so after the PR, the string looked like

talk to us on our %{discourse_link}

So the variable name changed from mailing_list_link to discourse_link. As you changed both the variable assignment in get_involved.html.erb and the variable usage in en.yml, things were fine in English. However, there is something we all missed: the fact that other locales exist. In other locales, like German for example, the locale string still looked like

unserer %{mailing_list_link} beizutreten

and now, we have an issue. If people used a German locale, Rails read the string and tried to look up a variable named mailing_list_link, couldn't find it, and thus threw an exception, resulting in the frontend 500'ing. And that's bad.

Now, there are a certain possible solutions here:

  1. Revert the original PR entirely. That's bad, because it would mean also reverting the changed string, so we'd be back where we started: pointing to closed mailing lists.
  2. Keep the content changes, but keeping the variable name at mailing_list_link. That'd be possible, but not favorable as we'd point to Discourse in a link named after the mailing list. This would be confusing.
  3. Drop all translations using mailing_list_link. This would have worked, since Rails would have been using the English fallback, but we would have lost all translations...
  4. Replace mailing_list_link with discourse_link in all translations. Sadly, webtranslateit has no global search-and-replace feature, so this would have been a bit of work.

All of those are bad, in one way or another. So instead, we picked option 5: keep the variable name, keep the changed texts, but also keep old translations working.

How? Well, simple. Remember our two locale strings:

talk to us on our %{discourse_link}
unserer %{mailing_list_link} beizutreten

and now look again at the diff in PR #127. In that PR, we re-defined the variable mailing_list_link, but set it to the new link target, and also have it use the new link text. Because the variable is now defined again, Rails no longer complains. Granted, "unserer Discourse beizutreten" is not proper German, but at least it works, and it may inspire someone to look at the translations, and maybe fix it. This workaround works because Rails does not care if a variable is unused (the English locale, for example, is obviously not using mailing_list_link), it only complains if you try to use an undefined variable.

Having this in place allows our translation community to gradually fix/adapt the translations, without us reverting to the "old" (and now, inappropriate) variable name or even reverting the contents.

Therefore, the line in the template file is actually not a duplicate, and in fact, with removing it, you'll now break the English site version. :) I hope things are clearer now that I've explained it in more detail, please let me know if not.


The PR looks fine, if you remove the removal of the workaround explained above, I'll merge it. :)

goobertron commented 5 years ago

Apologies; I got a bit confused there about what was needed and not in order to allow localisations to continue working. I've reinstated that line.

denschub commented 5 years ago

Thanks for the adjustment, PR merged and deployed. :)

denschub commented 5 years ago

Oh, lol. You broke it again!! https://bughunt.0b101010.services/share/issue/e4a458f3b75d4e6fb8f2a2e0a9c81f26/

goobertron commented 5 years ago

Urgh. Argh. That's the problem with not being able to test it.

If I force a rebased commit to the same branch, can you re-merge it? Or do I need to do something else?

denschub commented 5 years ago

Eh, too late. It was merged and deployed already. I pushed and deployed a fix: https://github.com/diaspora/diaspora-project-site/commit/c9142b8f806347cafcb9a986d3275ca3028f2630

Granted, I should have tested this. But oh well. :) Let's just say I wanted to verify that Sentry is going to catch our broken deployments.