Fix large attachment issue

[chavda-bhavik]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

• fix the large attachment not being sent

Why this change was needed? (You can also link to an open issue here)

• https://github.com/novuhq/novu/issues/1173

Other information:

• I implemented functionality that uploads attachments to S3 while trigger API is called, and while sending an email we get attachments from S3 and delete attachments after the email is sent. But, I have some questions @scopsy:

1. I tested it with S3_LOCALSTACK, do we need to check with Google Cloud & Azure? How?
2. Do we need test cases for it?
3. This change will make it require to include the LOCALSTACK_S3 setup for the production environment, so do we need to do anything for it?

[davidsoderberg]

Looks good, would be nice if we could add some tests :)

[chavda-bhavik]

@p-fernandez When this functionality will get merged, Or anything missing?

[p-fernandez]

@p-fernandez When this functionality will get merged, Or anything missing?

You missed to add the controlled error pattern to the Azure provider. Also would you mind to add tests for the error cases for the providers so we can have more confidence in the implementation? Also some crossed features changes that do not belong to the goal of your PR. Maybe you need to update your branch based on main one. You are quite close to finish this. It is not an easy one to do, thus the amount of revisions. 🙂

[chavda-bhavik]

@p-fernandez Sorry for inconvinence, Unwanted changes may happed while I merged main into my branch, because there were conflicts. And I added Error handling for Azure and GSP as well see here.

[p-fernandez]

@p-fernandez Sorry for inconvinence, Unwanted changes may happed while I merged main into my branch, because there were conflicts. And I added Error handling for Azure and GSP as well see here.

I misread the Azure implementation. Would you mind to do the tests for the error cases? 🙏🏻

[chavda-bhavik]

@p-fernandez Sorry for inconvinence, Unwanted changes may happed while I merged main into my branch, because there were conflicts. And I added Error handling for Azure and GSP as well see here.

I misread the Azure implementation. Would you mind to do the tests for the error cases? 🙏🏻

@p-fernandez Error test-cases refer to File should be null when it's not found on storage, right? Am I missing something?

[p-fernandez]

@p-fernandez Sorry for inconvinence, Unwanted changes may happed while I merged main into my branch, because there were conflicts. And I added Error handling for Azure and GSP as well see here.

I misread the Azure implementation. Would you mind to do the tests for the error cases? 🙏🏻

@p-fernandez Error test-cases refer to File should be null when it's not found on storage, right? Am I missing something?

Yes, you are right. It would give a lot of confidence providing those tests. For that mock the Cloud provider responses to throw an error and that the File is being set to null.

[chavda-bhavik]

@p-fernandez I added Test cases for Error Cases and also made attachment keys to follow this format,

const key = `${command.organizationId}/${command.environmentId}/${hat()}/${file.name}`;

I tested the functionality thoroughly with all storage providers, If you need any help in testing it, please let me know. Happy to help. 😊

[chavda-bhavik]

Hye @p-fernandez @scopsy Let me know if this PR is missing anything.

[p-fernandez]

@davidsoderberg @scopsy should we merge this?

[scopsy]

Just changed the base to release/0.9.0 and we can merge it