Skip to content

[APB-11045][APB-APB-11015] Success and FailedNonFixable emails#63

Open
gitwojciech wants to merge 47 commits intomainfrom
APB-11045
Open

[APB-11045][APB-APB-11015] Success and FailedNonFixable emails#63
gitwojciech wants to merge 47 commits intomainfrom
APB-11045

Conversation

@gitwojciech
Copy link
Copy Markdown
Contributor

No description provided.

paweldigital and others added 19 commits April 16, 2026 16:06
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
# Conflicts:
#	app/uk/gov/hmrc/agentregistration/shared/risking/IndividualRiskingResponse.scala
#	app/uk/gov/hmrc/agentregistrationrisking/model/IndividualForRisking.scala
#	test/uk/gov/hmrc/agentregistrationrisking/testsupport/testdata/TdApplicationForRisking.scala
#	test/uk/gov/hmrc/agentregistrationrisking/testsupport/testdata/TdIndividualForRisking.scala
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
# Conflicts:
#	app/uk/gov/hmrc/agentregistrationrisking/controllers/SubmitForRiskingController.scala
#	app/uk/gov/hmrc/agentregistrationrisking/testOnly/controllers/controllers/TestRiskingController.scala
#	test/uk/gov/hmrc/agentregistrationrisking/testsupport/testdata/TdSdesProxy.scala
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
@gitwojciech gitwojciech marked this pull request as draft April 29, 2026 14:35
paweldigital and others added 10 commits April 29, 2026 16:24
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
# Conflicts:
#	app/uk/gov/hmrc/agentregistrationrisking/controllers/SdesNotificationController.scala
#	app/uk/gov/hmrc/agentregistrationrisking/services/ApplicationStatusService.scala
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
# Conflicts:
#	app/uk/gov/hmrc/agentregistrationrisking/testOnly/controllers/controllers/TestRiskingController.scala
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
# Conflicts:
#	test/uk/gov/hmrc/agentregistrationrisking/controllers/SdesNotificationControllerSpec.scala
#	test/uk/gov/hmrc/agentregistrationrisking/repository/ApplicationForRiskingRepoSpec.scala
#	test/uk/gov/hmrc/agentregistrationrisking/scheduler/RiskingSchedulerInitializerSpec.scala
#	test/uk/gov/hmrc/agentregistrationrisking/services/SubscriptionServiceSpec.scala
#	test/uk/gov/hmrc/agentregistrationrisking/testsupport/testdata/TdAll.scala
gitwojciech and others added 12 commits May 5, 2026 11:15
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
Signed-off-by: pav <173694350+paweldigital@users.noreply.github.com>
# Conflicts:
#	app/uk/gov/hmrc/agentregistrationrisking/controllers/SdesNotificationController.scala
#	app/uk/gov/hmrc/agentregistrationrisking/repository/ApplicationForRiskingRepo.scala
#	app/uk/gov/hmrc/agentregistrationrisking/services/ApplicationStatusService.scala
#	app/uk/gov/hmrc/agentregistrationrisking/services/SubscriptionService.scala
@gitwojciech gitwojciech marked this pull request as ready for review May 7, 2026 13:29
# Conflicts:
#	app/uk/gov/hmrc/agentregistrationrisking/controllers/SdesNotificationController.scala
@platops-pr-bot
Copy link
Copy Markdown

)
).toFuture()

// def findReadyForSubscription(): Future[Seq[ApplicationForRisking]] = collection
Copy link
Copy Markdown
Contributor

@paweldigital paweldigital May 8, 2026

Choose a reason for hiding this comment

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

I was wondering what the purpose of this commented-out code is.

* limitations under the License.
*/

///*
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering if there’s any value in keeping these commented-out files, as they don’t seem to serve a purpose, or am I missing something? I have a small suggestion to consider removing them, and perhaps we could also avoid including similar unused or commented-out files in the future, if that’s not an issue of course.

)(using ExecutionContext)
extends RequestAwareLogging:

def sendEmail(emailInformation: EmailInformation)(using RequestHeader): Future[Unit] = emailConnector.sendEmail(emailInformation)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering if there’s any particular reason this method is exposed in the service. It seems to be used only internally, so I was thinking it might be worth considering whether it could be made private or inlined, especially since it relies on an underlying private connector to send emails. From an encapsulation perspective, it might help improve the structure of the code if we clean this up a bit, if that makes sense.

import play.api.libs.json.Json
import play.api.libs.json.OFormat

final case class EmailInformation(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we consider renaming EmailInformation to SendEmailRequest to better reflect that it represents the actual HTTP request used when calling the email microservice? It seems we already follow this naming convention in other connectors, so it might be good to stay consistent with that.

import play.api.libs.json.OFormat

final case class EmailInformation(
to: Seq[String],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering if it might be worth using a dedicated type for the email address, such as EmailAddress, instead of a plain String. It already seems to handle the necessary JSON serialization, and it could make the code more type-safe overall.

to: Seq[String] feels a bit unclear compared to to: Seq[EmailAddress], does it?


final case class EmailInformation(
to: Seq[String],
templateId: String,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we consider using a dedicated type for this as well? Since we only have a small set of email template values, it might make sense to model them as an enum. That way, we could reduce the risk of passing incompatible or unsupported values.

Comment on lines +27 to +28
eventUrl: Option[String] = None,
onSendUrl: Option[String] = None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering if those fields are still needed, as they don’t seem to be used anywhere. Maybe it would make sense to remove them to avoid maintaining code that isn’t actually in use?

Comment on lines +102 to +103
Filters.eq("isSubscribed", true),
Filters.eq("isEmailSent", false)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we consider using a FieldNames object to define these fields? It makes them easier to manage and help avoid typos in queries, especially since this has already happened in the project before.

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.

3 participants