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

WIP - add email confirmation #76

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

swdpcomputing
Copy link
Collaborator

No description provided.

@swdpcomputing swdpcomputing added help wanted Extra attention is needed question Further information is requested draft Not ready to merge labels Nov 22, 2024
docker-compose.yaml Outdated Show resolved Hide resolved
src/notification/Email-Service.js Outdated Show resolved Hide resolved
src/notification/handle-submission.js Outdated Show resolved Hide resolved
@swdpcomputing swdpcomputing force-pushed the tgc-337-email-confirmation branch 2 times, most recently from e0aabd2 to 9f0e4bf Compare December 4, 2024 08:49
@swdpcomputing
Copy link
Collaborator Author

Uploading Screenshot 2024-12-04 at 08.40.29.png…

@swdpcomputing swdpcomputing force-pushed the tgc-337-email-confirmation branch 2 times, most recently from 3fd9116 to a1ff39f Compare December 6, 2024 15:35
@swdpcomputing swdpcomputing removed help wanted Extra attention is needed question Further information is requested draft Not ready to merge labels Dec 6, 2024
@swdpcomputing swdpcomputing force-pushed the tgc-337-email-confirmation branch 6 times, most recently from 3ac0177 to bc28e84 Compare December 10, 2024 15:28
@swdpcomputing swdpcomputing force-pushed the tgc-337-email-confirmation branch 2 times, most recently from 72d6608 to 1163cf9 Compare December 11, 2024 09:20
@swdpcomputing swdpcomputing force-pushed the tgc-337-email-confirmation branch from 1163cf9 to c165247 Compare December 11, 2024 09:44
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this following the FCP container guidelines from here: https://defra.github.io/ffc-development-guide/development-patterns/containers/

We can introduce a new local docker compose config but there is already a dedicated docker-compose.override.yaml. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am following as far as I understand it. Happy to discuss.

Copy link
Collaborator

@Andrew-Folga Andrew-Folga Dec 12, 2024

Choose a reason for hiding this comment

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

As discussed yesterday let's merge it with the override one if possible.

@@ -4,7 +4,6 @@ services:
context: .
target: production
image: ffc-grants-eligibility-checker
# platform: ${PLATFORM}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is needed for ARM based architectures - why are we removing it here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's been replaced.

@@ -25,6 +25,7 @@
"start": "npm run build && nodemon ./src/index.js",
"start:debug": "node --inspect-brk=0.0.0.0 ./src/index.js",
"start:docker": "docker-compose -f docker-compose.yaml -f docker-compose.override.yaml up --build",
"start:docker:local": "docker-compose -f docker-compose.yaml -f docker-compose.local.yaml up --build",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above - why do we need separate docker compose config for local env - we already have the override config?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because the override is being run in sandbox, and we want to keep this only running on local machines.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Override is being run in both local and sandbox - there should be no local docker to avoid duplication.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there might have been an error during a rebase from main. We no longer have src/config/app.js but instead we have src/config/app/app-config.js.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah ok. Will adjust

})
}),

/**
Copy link
Collaborator

@Andrew-Folga Andrew-Folga Dec 11, 2024

Choose a reason for hiding this comment

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

XState has a dedicated services section for invoking long-running, asynchronous tasks (e.g., API calls, database queries, timers, etc.). Actions, on the other hand, are used for synchronous operations like logging, updating the context, or emitting additional events.

Use Services:

  1. For long-running tasks that need asynchronous handling (e.g., API calls, timers).
  2. When the task needs to determine the next state based on its success or failure.

Use Actions:

  1. For synchronous operations like logging, updating the context, or emitting additional events.
  2. When the operation doesn’t determine the next state.

Here is a useful table:

Aspect Services Actions
Purpose Handle asynchronous, potentially long-running tasks. Perform side effects or execute logic during state transitions.
Execution Timing Triggered when the machine enters a specific state (via invoke) or manually using send. Executed immediately during state transitions or on events.
Return Value Returns a promise or observable, used to determine the next state (onDone, onError). Returns nothing; performs tasks like logging, updating context, or dispatching events.
Integration Associated with states via invoke. Defined directly in actions and used in state transitions.

See: https://xstate.js.org/docs/guides/communication.html#the-invoke-property

@Andrew-Folga
Copy link
Collaborator

Could we get a small README section if relevant to mention few words about this new config? WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering whether we want to check in IDE specific files hmm.

@swdpcomputing swdpcomputing force-pushed the tgc-337-email-confirmation branch 4 times, most recently from fb874eb to 6044bf3 Compare December 13, 2024 11:41
Copy link

sonarcloud bot commented Dec 13, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
1 New Critical Issues (required ≤ 0)
1 New Major Issues (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@swdpcomputing swdpcomputing force-pushed the tgc-337-email-confirmation branch from 6044bf3 to 89b1957 Compare December 13, 2024 15:06
@swdpcomputing swdpcomputing force-pushed the tgc-337-email-confirmation branch from 89b1957 to 3e7d5a7 Compare December 13, 2024 15:43
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.

4 participants