Correct student names in confirmation email #967
Conversation
cesswairimu
force-pushed
the
feature/correct-student-name-application-email#955
branch
from
46d9658
to
da97ee1
Compare
|
@ramonh could you kindly look at this, it is working fine from my end, Thanks |
|
Hi @cesswairimu My apologies! I wasn't sure if you needed a review 😅 I'll have a look! Thank you! |
cesswairimu
changed the title
| expect(ActionMailer::DeliveryJob).to have_been_enqueued.with( | ||
| 'ApplicationFormMailer', 'submitted', 'deliver_now', | ||
| application: submitted_application, | ||
| student: students.second |
There was a problem hiding this comment.
Hi @cesswairimu Thank you so much for taking care of this and for your patience!
I'm curious, do you think there's a way that we can also verify that the content of the application in the email corresponds to the correct student? This is the issue we really want to avoid.
Thank you in advance!
There was a problem hiding this comment.
Will look at that @ramonh thanks for the feedback
|
Hey @cesswairimu, this is getting real close! Are you still interested in finishing it? Let us know if we can help, or if we can close this? |
|
Sorry @F3PiX Yes I am interested in finishing. I could use some help in testing. I tried implementing some tests but they are making another block of specs to fail. Should I push the file? or attach the snippet here so you can take a look? Thanks |
|
Hey @cesswairimu 👋 I think you can do whatever you prefer. We'll try to help either way 👍 |
|
Thanks @pgaspar just pushed the code. Thanks. |
| @@ -314,7 +314,8 @@ def assign_project(project) | |||
| end | |||
|
|
|||
| context 'when the application_draft is valid' do | |||
| let(:application_draft) { create(:application_draft, :appliable) } | |||
| let(:team) { create(:team) } | |||
There was a problem hiding this comment.
Hey @cesswairimu 👋 Gave this a quick look and it seems the issue is in this team you are creating. It needs to be an :applying_team for it to work well with the application draft.
But maybe instead of making the team work with create(:team, :applying_team) you could use the team that is created for you in the application_draft. Our factory already creates a team, so you can access it in your tests and use applying_team.team.students.first, for example. This way you could remove the team you created.
cesswairimu
force-pushed
the
feature/correct-student-name-application-email#955
branch
2 times, most recently
from
1e40d60
to
f4acc3b
Compare
There was a problem hiding this comment.
Thank you so much @cesswairimu for your changes! Looking great so far!
I have one suggestion for making this completely error-proof. Not sure how to go about this but I'll take a look and see if I can help 😊
| application: submitted_application, | ||
| student: application_draft.team.students.first | ||
| ) | ||
| expect(ActionMailer::DeliveryJob).to have_been_enqueued.with( |
There was a problem hiding this comment.
This looks great so far, thank you @cesswairimu!
My only concern is that issue #955 cropped up due to the fact that the students' names were assigned to the emails but the contents of the application were being assigned to the wrong student.
Is there a way we can test the contents and make sure that the correct application information is assigned to the correct student?
|
Gotcha @ramonh will take a look see what I can find |
|
Heads up: Changes in #1007 has effect on this PR. See bc26170 (and its commits message). @cesswairimu To make sure that your changes still work, you can pull the remote master branch into your local master branch, and then rebase. Or, if you don't want to rebase, go to your local master, pull the remote master; and open a new PR/branch. |
|
👍 Thanks @F3PiX |
So as to investigate t he bug that find solution
cesswairimu
force-pushed
the
feature/correct-student-name-application-email#955
branch
from
f4acc3b
to
3eac6fe
Compare
Restore email sending code
So as to investigate the bug and find solution
Related issue #955