Fix/sofialink

[lodewiges]

edited the link in envoriment so the staging site links to stagingstreep.csvalpha.nl instead of streep.csvalpha.nl

[DrumsnChocolate]

we are now using two variables here, is that wise?

[lodewiges]

No, that is not the ideal situation. but is better then the situation now

[lodewiges]

okay, I did some more research it is possible to use one variable however we need to rewrite logic in the envirment.js file and change the docker files on the server. I think this has too high a risk of breaking something to be worth it, if you don't agree we can look into the possibility of doing it in an other manner.

[DrumsnChocolate]

Okay so my point is that environment and deployTarget seem to be used in similar context in similar ways. Why do we distinguish between them? Can you explain?

There are currently four places where config.environment is used, and I don't see why we couldn't use config.deployTarget instead. How are these two variables really different?

[lodewiges]

I didn't write the original code, so I can't explain it with certainty. I think the general idea was that we have three environments: test, development, and production. All three are different environments and sometimes require different dependencies.

within the production environment we have 2 deploy targets: production and staging, it is confusing that we use production twice. the difference between these deploy targets are configurations they should use the same dependencies.

This leads me to think the second suggestion is the better of the 2

[DrumsnChocolate]

okay that does clear things up a bit, I'm still confused about some stuff but I'll try to formulate my thoughts:

• My intuition tells me that direct access to deployTarget is not something the menu-sidebar.js needs to be responsible for. It should not have to think too much about what the sofiaUrl is, just like router.js does not have to think too hard about what rootUrl is. So, my recommendation is to move the logic for determining the sofiaUrl out of menu-sidebar.js and into environment.js.
• I agree that using production twice is confusing. I'm not sure what the production environment refers to, but the other two should be clear. I think that the production environment might really mean a real-world environment? There's a bunch of words for it, build could also be an alternative. Then you would have test, development, build.
• You say there are 2 deploy targets, I would say there is an extra implicit deployTarget: local. You can recognize this in environment.js by seeing that ENV.clientId has a fall back value for when deployTarget is neither production or staging. We don't need to write this down because it requires extra work
• I feel like the sofiaUrl should not depend on environment, but on deployTarget. In general, I think most config values should not depend on what environment we're running in, but what target we're running on. The environment feels more like a 'mode' of running, while deployTarget feels like a 'place'.
  However, there are still things we definitely should turn off or on based on which 'mode' we're in. Two examples are sentry and apparently service workers (no clue what those are).
  Like you mentioned, different environments may require different dependencies.

TLDR

• I identify three envs: test, development, production (or build, we could rename to that)
• I identify three deploy targets: local, staging, production
• we should define config.sofiaUrl, and determine its value in environment.js where config.rootUrl is also defined.
• we should stop using config.environment to determine config.sofiaUrl: it should only really depend on config.deployTarget i.e. where is the code running.

[DrumsnChocolate]

(yes I'm aware that we're going beyond the scope of this PR, I hope you don't mind. if you prefer to get staging working first, and discuss details like this later, I can approve this PR and you can make my suggested changes in another PR)

[lodewiges]

Okay, I agree with the first point you are making.

As far I understand this is correct "I think that the production environment might really mean a real-world environment" Build would be a better alternative. I can make an issue for this.

However I don't agree with the third point, there are 2 deploy targets we deploy to using GitHub actions. We dont deploy to local or test but we do need a variable for clientID otherwise we are unable to connect to amber-api.

I have no problem with discussing it thoroughly, I learn alot doing this so i am happy.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 13.80%. Comparing base (26f13a0) to head (24b7017).
Report is 1 commits behind head on staging.

Additional details and impacted files

@@             Coverage Diff             @@
##           staging     #913      +/-   ##
===========================================
+ Coverage    13.79%   13.80%   +0.01%     
===========================================
  Files          450      450              
  Lines         3067     3063       -4     
===========================================
  Hits           423      423              
+ Misses        2644     2640       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.