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

add email event UI triggers for resend option in emails #3017

Open
wants to merge 23 commits into
base: minor
Choose a base branch
from

Conversation

monrostar
Copy link
Contributor

@monrostar monrostar commented Aug 17, 2024

Description

#3016

Breaking changes

Does this PR include any breaking changes we should be aware of?

Screenshots

You can add screenshots here if applicable.

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

👍 Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

Copy link

vercel bot commented Aug 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Aug 28, 2024 11:37am

@monrostar monrostar changed the title Feat/add email event UI triggers WIP add email event UI triggers Aug 17, 2024
github-actions bot and others added 3 commits August 22, 2024 08:52
changed "The desired shipping method's id is the" to "The desired shipping method's id is then"
…s and create component

Signed-off-by: Eugene Nitsenko <[email protected]>
ctx,
new Injector(this.moduleRef),
entity,
args.input.arguments,
Copy link
Member

Choose a reason for hiding this comment

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

Note that there is a case that needs to be handled here for arguments of the type ID. Since Vendure has the concept of ID encoding/decoding, we also need to ensure that any ID arguments are correctly decoded before passing them to the createEvent function.

This can be done by injecting the ConfigurableOperationCodec helper and calling decodeConfigurableOperationIds() , passing the handler and args.

The function mutates the arg.value for any IDS. You can see an example of usage here:

this.configurableOperationCodec.decodeConfigurableOperationIds(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a value parser in this place but there seems to be no ID decode there...

],
description: [
{
value: 'Order confirmation can be send only for specific reasons.',
Copy link
Member

Choose a reason for hiding this comment

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

"can be sent"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

entityType: Order,
label: [
{
value: 'Order confirmation.',
Copy link
Member

Choose a reason for hiding this comment

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

drop the .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@michaelbromley
Copy link
Member

Also, make the PR against the minor branch please 👍

@monrostar monrostar changed the base branch from master to minor August 27, 2024 11:35
Signed-off-by: Eugene Nitsenko <[email protected]>
Signed-off-by: Eugene Nitsenko <[email protected]>
Signed-off-by: Eugene Nitsenko <[email protected]>
Signed-off-by: Eugene Nitsenko <[email protected]>
Signed-off-by: Eugene Nitsenko <[email protected]>
@monrostar
Copy link
Contributor Author

What I didn't finish:

  1. Only a temporary UI for the Order page is made. Not added EmailEventUI to customer
  2. Not added Permissions, maybe it is possible to make permissions only for EmailEventResend...?
  3. I have done args parsing, but I wanted to discuss how to add ID decode since EmailPlugin has no properties in VendureConfig and maybe you can find a better solution
  4. No Errors have been added to the email-event.resolver.ts

@monrostar monrostar changed the title WIP add email event UI triggers add email event UI triggers for resend option in emails Aug 28, 2024
@michaelbromley
Copy link
Member

Status update from my side:

I pulled this PR locally and tested it out. Basically it looks good but I have a few points I want to spend more time thinking about when I am a little less busy:

  • We discussed extracting the UI parts to the EmailPlugin to avoid the coupling. Architecturally this would be correct. DX-wise it would mean more complexity to expose this functionality in the UI.
  • In that regard it would be nice to be able to expose this functionality out-of-the-box without requiring to compile the Admin UI.

But in order to bundle this functionality into the Admin UI I'd really only want to do that if the functionality is not specific to the EmailPlugin.

So I'm wondering whether in here there is a more generic concept such as "event replay" which would be more broadly applicable, and then the email re-send is a specific implementation of that.

@monrostar
Copy link
Contributor Author

Status update from my side:

I pulled this PR locally and tested it out. Basically it looks good but I have a few points I want to spend more time thinking about when I am a little less busy:

  • We discussed extracting the UI parts to the EmailPlugin to avoid the coupling. Architecturally this would be correct. DX-wise it would mean more complexity to expose this functionality in the UI.
  • In that regard it would be nice to be able to expose this functionality out-of-the-box without requiring to compile the Admin UI.

But in order to bundle this functionality into the Admin UI I'd really only want to do that if the functionality is not specific to the EmailPlugin.

So I'm wondering whether in here there is a more generic concept such as "event replay" which would be more broadly applicable, and then the email re-send is a specific implementation of that.

Do you need help integrating such functionality? And it would be cool to know when we can get such a feature

@michaelbromley
Copy link
Member

Just an update as I am going through old PRs this morning: this is still on our "under consideration" lane in the roadmap. For now we have other higher priority features we want to ship but I'm leaving this open because it is fundamentally a good feature.

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