Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

notifications funcitonality added #GCPActive #62

Merged
merged 10 commits into from
Nov 20, 2023
Merged

Conversation

acn-sbuad
Copy link
Contributor

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 ivarne self-requested a review November 15, 2023 15:41
Copy link
Member

@ivarne ivarne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the difficulties we currently have because LocalTest isn't in sync with all the repositories it copies code from, I think this is a great opportunity to organise the code a little differently.

My suggestion would be something like

.src/
    Altinn/
        Notifications/
            - All code copied from Notifications (except `.csproj`, ...?)
            - Any deliberate changes is clearly marked with a comment 
              (Ideally no changes with but use `#if LOCALTEST` to maintain both versions in the same code instead)
    Services/
        Notifications/
            LocaltestOrderRepository.cs

@ivarne
Copy link
Member

ivarne commented Nov 15, 2023

@acn-sbuad
Copy link
Contributor Author

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
Copy link
Member

ivarne commented Nov 16, 2023

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
Copy link
Member

ivarne commented Nov 16, 2023

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
Copy link
Contributor Author

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 ;)

@acn-sbuad acn-sbuad requested a review from ivarne November 16, 2023 14:38
Copy link
Member

@ivarne ivarne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@ivarne ivarne merged commit be99926 into main Nov 20, 2023
1 check passed
@ivarne ivarne deleted the feature/notifications branch November 20, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants