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.

Sign up for GitHub

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

Jump to bottom

RHCLOUD-29047 | feature: implement email sending settings #103

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

RHCLOUD-29047 | feature: implement email sending settings #103

wants to merge 2 commits into from

Conversation

MikelAlejoBR
Copy link
Member

The back office proxy supports skipping the user resolution and overriding the email's sender and the email's default recipients, and the idea of these changes is for MBOP to support the same things too.

Jira ticket

[RHCLOUD-29047]

@MikelAlejoBR MikelAlejoBR force-pushed the RHCLOUD-29047-implement-email-sending-settings branch 2 times, most recently from 78e4954 to 1b3021c Compare November 13, 2023 16:24
@MikelAlejoBR
Copy link
Member Author

/retest

6 similar comments
@MikelAlejoBR
Copy link
Member Author

/retest

@MikelAlejoBR
Copy link
Member Author

/retest

@MikelAlejoBR
Copy link
Member Author

/retest

@lpichler
Copy link

/retest

@MikelAlejoBR
Copy link
Member Author

/retest

@dagbay-rh
Copy link
Contributor

/retest

@dagbay-rh dagbay-rh self-requested a review December 7, 2023 18:42
dagbay-rh
dagbay-rh reviewed Dec 7, 2023
View reviewed changes
Copy link
Contributor

@dagbay-rh dagbay-rh left a comment

Choose a reason for hiding this comment

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

all the new logic looks good to me! only thing I am wondering about: are there tests we can add/update to cover the new logic in send_email.go? im not very familiar with mbop so apologies not knowing the tests setup

@dagbay-rh
Copy link
Contributor

the pr build is failing due to:
12:50:20 requests.exceptions.HTTPError: 400 Client Error: Bad Request for url: https://ee-xhhvb4rb-auth.apps.c-rh-c-eph.8p0c.p1.openshiftapps.com/auth/realms/redhat-external/protocol/openid-connect/token

seems related to bonfire stuffs happening. our unit tests are passing so I think this should be ok to ignore for now. not totally sure who would know more about these builds tho, would be nice to see if we can get them passing or fix them etc

@MikelAlejoBR
Copy link
Member Author

all the new logic looks good to me! only thing I am wondering about: are there tests we can add/update to cover the new logic in send_email.go? im not very familiar with mbop so apologies not knowing the tests setup

I really have no idea. Maybe I can try to build a mock for the SendEmails function?

@gwenneg
Copy link
Member

gwenneg commented Mar 22, 2024

Hi everyone! Any chance this could be merged soon?

@MikelAlejoBR
Copy link
Member Author

Hello,

I need to take a look at those tests and implement them. I'll get to it.

@MikelAlejoBR
feature: implement email sending settings
46aae06 
The back office proxy supoports skipping the user resolution and
overriding the email's sender and the email's default recipients, and
the idea of these changes is for MBOP to support the same things too.

RHCLOUD-29047
@MikelAlejoBR MikelAlejoBR force-pushed the RHCLOUD-29047-implement-email-sending-settings branch 2 times, most recently from c8f5b8a to a45349c Compare March 25, 2024 15:42
@MikelAlejoBR
Copy link
Member Author

I've added a few tests as requested @dagbay-rh

@MikelAlejoBR MikelAlejoBR force-pushed the RHCLOUD-29047-implement-email-sending-settings branch 3 times, most recently from 8168e13 to bf71a33 Compare March 25, 2024 16:41
@MikelAlejoBR MikelAlejoBR force-pushed the RHCLOUD-29047-implement-email-sending-settings branch from bf71a33 to fd8e70c Compare March 25, 2024 16:44
@MikelAlejoBR
Copy link
Member Author

/retest

2 similar comments
@MikelAlejoBR
Copy link
Member Author

/retest

@abaiken
Copy link
Member

abaiken commented Apr 9, 2024

/retest

@abaiken
Copy link
Member

abaiken commented Apr 9, 2024

I am fine with merging this 👍 The logic looks good to me! I think the last PR we merged was also failing the PR check so i do not think its related to your PR.

@gwenneg
Copy link
Member

gwenneg commented Apr 10, 2024 •
edited
Loading

Let's make sure this PR won't create any issues in the special environment before it is merged.

Until then...

image

cc @MikelAlejoBR

@MikelAlejoBR MikelAlejoBR marked this pull request as draft April 10, 2024 13:58
@MikelAlejoBR
Copy link
Member Author

@MikelAlejoBR test it in ephemeral

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dagbay-rh dagbay-rh dagbay-rh left review comments

@abaiken abaiken abaiken approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MikelAlejoBR @lpichler @dagbay-rh @gwenneg @abaiken