Add privacy policy link as env var

[awarn]

Description

Simply adds env var for the link

Related issues

Resolves #2116

[richardolsson]

Nice work! I'd like for the use of the environment variable to be moved out of messageIds.ts but other than that this looks like a good solution to me!

[richardolsson]

I'm afraid we can't do this here, because the default text needs to be static.

Instead of putting the environment variable here, can you put the same || logic wherever feat.surveys.surveyForm.policy.link is used?

Or maybe remove the translated message altogether and we can just require all instances to have this environment variable to function correctly?

[awarn]

Yeah, having multiple places the variable is set without a real use case for it seems weird. If it doesn't change with the translation, then I say we put it only in environment. And we can still fall back on the original, right?

[richardolsson]

I think we should fall back to the translation, like this (wherever the message is used today):

const privacyUrl = process.env.ZETKIN_PRIVACY_POLICY_LINK || messages.surveyForm.policy.link();

Now there's another potential problem here which is that the ZETKIN_PRIVACY_POLICY_LINK variable will not be available on the frontend. This is solved elsewhere by explicitly adding the environment variable to the Environment that can then be accessed by useEnv().

[richardolsson]

The reason why I prefer using the translation as the fallback is that for some instances (e.g. the main Zetkin instance used by most organizations) it is still useful to be able to have different translated versions of the same privacy policy, and for that the messages is a good solution.

[awarn]

Aah, okay, yes. Then I understand the priority here, I think. I'll make the change.

[richardolsson]

Nice work! I have looked at the code and tried running the build locally with and without the environment variable to test it's effect. I left a comment below about the intrinsic problems of this solution, but after thinking about it more closely I realized there isn't really a (small) better solution.

One thing that we could do is write a Playwright test to make sure the link works as expected at runtime. Are you familiar and comfortable with Playwright tests and want to have a go at it, or do you want me to?

[richardolsson]

This works, because this is a React Server Component, i.e. it's rendered on the server where environment variables are available. If this component is ever rendered by the client, it will not work.

In those cases, we need to use useEnv() instead. But that wouldn't work in a RSC (like this one) because it relies on context.

So this is the right way to do it, but also error-prone because it's easy to forget to change to useEnv() if this component is ever modified to be client-rendered. I'm mentioning it here for future forensics into potential bugs.

[richardolsson]

I think the tests you've added provide sufficiently good coverage to feel comfortable releasing this into the wild.

The only problem I see with the "raw" approach to updating process.env at the beginning of each test is that it might create flaky tests in the future (if other tests also rely on this environment variable). But if we end up there, we can look into tweaking it then.

[richardolsson]

I believe(?) there are other ways of mocking environment variables in jest, that makes sure they are reset after a test runs.