Update resource before to process email so that partner invitation emails have correct body #3027

italomatos · 2022-07-13T10:54:19Z

Resolves #2985

Description

This PR has goal to fix email message when a partner agency is invited.

Type of change

Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

create a new partner
invite them

Screenshots

before

after

italomatos · 2022-07-13T11:13:50Z

app/mailers/custom_devise_mailer.rb

@@ -7,6 +7,8 @@ def subject_for(key)
    # Replace the invitation instruction subject for partner users
    # that were invited by other partner users.

+    resource.reload
+
    if resource.is_a?(PartnerUser) && resource.id == resource.partner.primary_user&.id


The problem is partner primary user is nil, when test in this condition.

Before check this condition is necessary update resource data.

cielf

Hi Italo,
Could you also add a test for this?

italomatos · 2022-07-19T22:38:16Z

Hi Italo,
Could you also add a test for this?

Hi @cielf I'm trying to identify where I will create a test.
Because this method belongs to devise_invitable

cielf · 2022-07-20T14:15:24Z

Hi Italo,
Could you also add a test for this?

Hi @cielf I'm trying to identify where I will create a test. Because this method belongs to devise_invitable

Myself, I would put the test in spec/mailers/custom_devise_mailer_spec.rb, which doesn't exist yet.

cielf

Can you test that the right text is shown? (That's what the initial problem was)

italomatos · 2022-07-20T21:48:33Z

Can you test that the right text is shown? (That's what the initial problem was)

@cielf Sure, actually I tested and attached print in PR description ( before fix and after fix).

italomatos · 2022-07-20T21:52:34Z

Hi Italo,
Could you also add a test for this?

Hi @cielf I'm trying to identify where I will create a test. Because this method belongs to devise_invitable

Myself, I would put the test in spec/mailers/custom_devise_mailer_spec.rb, which doesn't exist yet.

Hi @cielf

Actually I had created this spec, so, I didnt like put business solution in Mailer file. So, I moved reload to InviteService, and created a test to cover that. But I aggree that We need to have spec to CustomDeviseMailer, but I didnt submit in this PR because it is not sollution to this problem. What do you think that I create a new PR to create spec to CustomDeviseMailer ?

cielf · 2022-07-20T22:31:58Z

I'd like to see tests showing that the issue is fixed -- that the right text comes out in each case, if that can be done without too much pain. We do check that the right text is sent in other mailer specs.

italomatos · 2022-07-21T02:10:58Z

spec/requests/partners_requests_spec.rb

+      expect(ActionMailer::Base.deliveries.last.subject).to eq("You've been invited to be a partner with #{@organization.name}")
+      expect(ActionMailer::Base.deliveries.last.html_part.body).to include("You've been invited to become a partner organization with <strong>#{@organization.name}!</strong>")


Hi @cielf , this test check subject and body, what do you think?

@italomatos I think we don't need to test the text in both the mailer spec and the requests spec -- following the patterns that I see in the current tests, I would use the mailer spec for testing the text. Does that make sense to you?

cielf

Hi @italomatos -- a couple of questions/comments:
1/ there is already a partners_user in factories/partners/user -- does that not serve the purpose?
2/ Can we check the body text in the custom_devise_mailer_spec?
3/ I don't think we need to also check the text in the requests spec if we can check the body in the custom_devise_mailer_spec

cielf · 2022-07-21T14:32:46Z

spec/requests/partners_requests_spec.rb

+      expect(ActionMailer::Base.deliveries.last.subject).to eq("You've been invited to be a partner with #{@organization.name}")
+      expect(ActionMailer::Base.deliveries.last.html_part.body).to include("You've been invited to become a partner organization with <strong>#{@organization.name}!</strong>")


@italomatos I think we don't need to test the text in both the mailer spec and the requests spec -- following the patterns that I see in the current tests, I would use the mailer spec for testing the text. Does that make sense to you?

italomatos · 2022-07-22T01:33:21Z

Hi @cielf

factories/partners/user not work, because this factory is object from Partners::User , and the implementation is considering a instance of PartnerUser . I think a little bit confusing these models. But there is a comment in PartnerUser , the explain the reason to created it.
Sure, I'm modifying to check body text too.
make sense.

cielf · 2022-07-25T15:47:09Z

I agree that the model is confusing -- we're in the midst of consolidating the user models -- see Daniel's PR 3013: Consolidate users #3050.

cielf · 2022-07-25T15:50:26Z

Indeed, I think it might be a good idea to hold off on this one, rebase and rework according to his changes when it's been merged. It's not a high priority change. What do you think?

italomatos · 2022-07-26T01:53:21Z

Indeed, I think it might be a good idea to hold off on this one, rebase and rework according to his changes when it's been merged. It's not a high priority change. What do you think?

ok, make sense

edwinthinks · 2022-07-28T12:22:55Z

@italomatos just a quick update on the title so we know this is blocked :)

This reverts commit 890b29c.

edwinthinks · 2022-07-31T11:13:59Z

@italomatos the PR for merging the user models #3050 have been completed! I think you can continue working on this :)

italomatos · 2022-07-31T17:15:13Z

@italomatos the PR for merging the user models #3050 have been completed! I think you can continue working on this :)

Hi @edwinthinks I did rebase, and this PR is ready to review :)

edwinthinks

Thanks @italomatos a few minor suggestions and once those have been addressed I think we a good to go!

edwinthinks · 2022-08-04T01:05:42Z

app/services/partner_invite_service.rb

+    user.reload
+    user.deliver_invitation


One minor suggestion, can you add a comment to explain why this was added? I sense it will be unclear to future maintainers about why this was added.

Hi @edwinthinks, I agree. Is it ok? Thanks

edwinthinks · 2022-08-04T01:06:02Z

app/views/users/mailer/invitation_instructions.html.erb

+                    <% is_primary_partner = @resource.id == @resource.partner&.primary_user&.id %>
+                    <p>Hello <%= @resource.email %></p>
+                    <% if @resource.partner.present? && is_primary_partner %>
+                      <p>You've been invited to become a partner organization with <strong><%= organization.name %>!</strong></p>


Suggested change

You've been invited to become a partner organization with <%= organization.name %>!

You've been invited to become a partner with <%= organization.name %>!

edwinthinks · 2022-08-04T01:06:42Z

app/views/users/mailer/invitation_instructions.html.erb

+                    <p>Feel free to ignore this email if you are not interested or if you feel it was sent by
+                        mistake.</p>


Suggested change

Feel free to ignore this email if you are not interested or if you feel it was sent by

mistake.

Feel free to ignore this email if you are not interested or if you feel it was sent by mistake.

…on user object needs to reload before send email

edwinthinks · 2022-08-06T17:44:02Z

Hey @italomatos -- I think there is just a few specs to fix. Probably from the changes suggested.

…tfix/2985-wrong-message-email-to-partner-organization

…rganization

italomatos commented Jul 13, 2022

View reviewed changes

edwinthinks requested a review from cielf July 17, 2022 15:06

italomatos force-pushed the hotfix/2985-wrong-message-email-to-partner-organization branch from 228706f to 1b53c37 Compare July 17, 2022 15:34

cielf reviewed Jul 18, 2022

View reviewed changes

italomatos force-pushed the hotfix/2985-wrong-message-email-to-partner-organization branch 2 times, most recently from 7c7b232 to 2863fc1 Compare July 20, 2022 01:00

italomatos requested a review from cielf July 20, 2022 01:01

italomatos force-pushed the hotfix/2985-wrong-message-email-to-partner-organization branch from 2863fc1 to d84ef30 Compare July 20, 2022 01:04

cielf reviewed Jul 20, 2022

View reviewed changes

italomatos requested a review from cielf July 20, 2022 22:08

italomatos commented Jul 21, 2022

View reviewed changes

italomatos force-pushed the hotfix/2985-wrong-message-email-to-partner-organization branch 2 times, most recently from 1f2518c to eceded4 Compare July 21, 2022 08:50

cielf reviewed Jul 21, 2022

View reviewed changes

italomatos force-pushed the hotfix/2985-wrong-message-email-to-partner-organization branch from eceded4 to 5558e84 Compare July 22, 2022 01:40

italomatos requested a review from cielf July 22, 2022 01:41

edwinthinks changed the title ~~update resource before to process email~~ [BLOCKED] Update resource before to process email so that partner invitation emails have correct body Jul 28, 2022

italomatos added 3 commits July 31, 2022 00:31

remove rspec-retry

473b9f9

Revert "remove rspec-retry"

bbf0677

This reverts commit 890b29c.

add spec to partner invite service and hotfix to send correct email

49cf2f1

add spec to check subject and body invite

f6ba45f

edwinthinks changed the title ~~[BLOCKED] Update resource before to process email so that partner invitation emails have correct body~~ Update resource before to process email so that partner invitation emails have correct body Jul 31, 2022

refactory to use user model

b549a09

italomatos force-pushed the hotfix/2985-wrong-message-email-to-partner-organization branch from 5558e84 to b549a09 Compare July 31, 2022 12:57

italomatos changed the title ~~Update resource before to process email so that partner invitation emails have correct body~~ WIP - Update resource before to process email so that partner invitation emails have correct body Jul 31, 2022

italomatos marked this pull request as draft July 31, 2022 12:59

fix partner invite service spec

8510850

italomatos force-pushed the hotfix/2985-wrong-message-email-to-partner-organization branch from 7368644 to e598b34 Compare July 31, 2022 17:11

italomatos changed the title ~~WIP - Update resource before to process email so that partner invitation emails have correct body~~ Update resource before to process email so that partner invitation emails have correct body Jul 31, 2022

italomatos marked this pull request as ready for review July 31, 2022 17:11

italomatos force-pushed the hotfix/2985-wrong-message-email-to-partner-organization branch from e598b34 to 52f3acd Compare July 31, 2022 17:32

fix invitation_instructions to partner users

8e0d1cb

italomatos force-pushed the hotfix/2985-wrong-message-email-to-partner-organization branch from 52f3acd to 8e0d1cb Compare August 1, 2022 11:35

edwinthinks reviewed Aug 4, 2022

View reviewed changes

fix invitation instructions texts and add comment to explain the reas…

fbcd32a

…on user object needs to reload before send email

fix spec expect message

ecff9f8

italomatos requested a review from edwinthinks August 6, 2022 22:16

Italo Matos and others added 2 commits August 6, 2022 19:52

Merge branch 'main' of github.com:italomatos/human-essentials into ho…

54f83b9

…tfix/2985-wrong-message-email-to-partner-organization

Merge branch 'main' into hotfix/2985-wrong-message-email-to-partner-o…

8db5edd

…rganization

edwinthinks merged commit d36a88b into rubyforgood:main Aug 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update resource before to process email so that partner invitation emails have correct body #3027

Update resource before to process email so that partner invitation emails have correct body #3027

italomatos commented Jul 13, 2022

italomatos Jul 13, 2022 •

edited

Loading

cielf left a comment

italomatos commented Jul 19, 2022

cielf commented Jul 20, 2022 •

edited

Loading

cielf left a comment

italomatos commented Jul 20, 2022

italomatos commented Jul 20, 2022

cielf commented Jul 20, 2022 •

edited

Loading

italomatos Jul 21, 2022

cielf Jul 21, 2022

cielf left a comment

cielf Jul 21, 2022

italomatos commented Jul 22, 2022

cielf commented Jul 25, 2022

cielf commented Jul 25, 2022 •

edited

Loading

italomatos commented Jul 26, 2022

edwinthinks commented Jul 28, 2022

edwinthinks commented Jul 31, 2022

italomatos commented Jul 31, 2022

edwinthinks left a comment

edwinthinks Aug 4, 2022

italomatos Aug 5, 2022

edwinthinks Aug 4, 2022

edwinthinks Aug 4, 2022

edwinthinks commented Aug 6, 2022

		expect(ActionMailer::Base.deliveries.last.subject).to eq("You've been invited to be a partner with #{@organization.name}")
		expect(ActionMailer::Base.deliveries.last.html_part.body).to include("You've been invited to become a partner organization with <strong>#{@organization.name}!</strong>")

	<p>You've been invited to become a partner organization with <strong><%= organization.name %>!</strong></p>
	<p>You've been invited to become a partner with <strong><%= organization.name %>!</strong></p>

		<p>Feel free to ignore this email if you are not interested or if you feel it was sent by
		mistake.</p>

Update resource before to process email so that partner invitation emails have correct body #3027

Update resource before to process email so that partner invitation emails have correct body #3027

Conversation

italomatos commented Jul 13, 2022

Description

Type of change

How Has This Been Tested?

Screenshots

before

after

italomatos Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

cielf left a comment

Choose a reason for hiding this comment

italomatos commented Jul 19, 2022

cielf commented Jul 20, 2022 • edited Loading

cielf left a comment

Choose a reason for hiding this comment

italomatos commented Jul 20, 2022

italomatos commented Jul 20, 2022

cielf commented Jul 20, 2022 • edited Loading

italomatos Jul 21, 2022

Choose a reason for hiding this comment

cielf Jul 21, 2022

Choose a reason for hiding this comment

cielf left a comment

Choose a reason for hiding this comment

cielf Jul 21, 2022

Choose a reason for hiding this comment

italomatos commented Jul 22, 2022

cielf commented Jul 25, 2022

cielf commented Jul 25, 2022 • edited Loading

italomatos commented Jul 26, 2022

edwinthinks commented Jul 28, 2022

edwinthinks commented Jul 31, 2022

italomatos commented Jul 31, 2022

edwinthinks left a comment

Choose a reason for hiding this comment

edwinthinks Aug 4, 2022

Choose a reason for hiding this comment

italomatos Aug 5, 2022

Choose a reason for hiding this comment

edwinthinks Aug 4, 2022

Choose a reason for hiding this comment

edwinthinks Aug 4, 2022

Choose a reason for hiding this comment

edwinthinks commented Aug 6, 2022

italomatos Jul 13, 2022 •

edited

Loading

cielf commented Jul 20, 2022 •

edited

Loading

cielf commented Jul 20, 2022 •

edited

Loading

cielf commented Jul 25, 2022 •

edited

Loading