search

Altinn / app-localtest

Solution for testing apps locally on your own machine
8 stars 14 forks source link

notifications funcitonality added #GCPActive #62

Closed acn-sbuad closed 10 months ago

acn-sbuad commented 10 months ago

Description

Duplicated functionality for generating a email notification order. Order is persisted to disk in a sub directory to LocalTestingStorageBasePath

full notification order is persisted as a json document.

altinn/altinn-notifications#239

ivarne commented 10 months ago
acn-sbuad commented 10 months ago

We can definetly group all code in a single folder

I'm not sure I want to introduce #if LOCALTEST in our original code base. Localtest is closely related to Altinn Apps, and supports apps in running locally, but has nothing to do with the local development of Altinn Platform products. Testing notifications locally would for instance have nothing to do with localtest making the notation if LOCALTEST confusing for developers and others that read the code.

I believe it complicates the code and introduces a potential risk for bugs / security issues. The difference is mainly in the controllers and repository files, whereas the services with the actual business logic are copied directly without change.

ivarne commented 10 months ago

I'm not sure about the #if LOCALTEST thing either. It was just a thought I had for those cases where a custom service with a localtest implementation can't be used. I don't think there is a huge risk that anyone will define LOCALTEST in the upstream code causing security issues.

Most of the code that has been copied into app-localtest has diverged quite a lot from the code that it tries to represent a local mock for. In authentication I found a bug that caused a policy.xml to specify that something worked in localtest, but it didn't work in tt02. Just copying in the new code didn't work because it was way too hard for me to track down and reapply the original modifications. Maintaining in one place is a little extra burden there, but maintaining separate code bases isn't exactly easy that either, and as you say: "services with the actual business logic are copied directly without change".

ivarne commented 10 months ago

The copy operations seems to have changed the namespace to Localtest. and removed nullable annotations? Could you revert those changes (possibly adding #nullable enable on top of all files to ease the transition later?)

acn-sbuad commented 10 months ago

Authentication and authorization might be the biggest challenges with localtest I think, but those are complex areas in the general platform component as well. We would need to agree on wether or not localtest should only cover the basics and be a mock with minimum required functionality for an app to run locally or if we actually want to be a copy of the platform component. The latter would requrire quite a lot of extra work, but with potential limited reward is my assumption.

All code moved into a designated notifications folder now ;)