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

#90 - send e-mail when provider is added to or removed from favorite city #197

Conversation

JakubKermes
Copy link
Collaborator

@JakubKermes JakubKermes commented Jan 10, 2024

image

This should close #90

@JakubKermes JakubKermes requested a review from a team as a code owner January 10, 2024 19:20
@JakubKermes JakubKermes linked an issue Jan 10, 2024 that may be closed by this pull request
Copy link
Contributor

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

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

  1. Comments are unnecessary here.
  2. I don't receive e-mail, when new provider is added to favorite city
  3. In the codebase I see word "favorite" and "favourite" - we should unify that.
  4. I think that we can use here Notification - https://laravel.com/docs/10.x/notifications

example in toby: blumilksoftware/toby#54

Copy link
Contributor

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

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

Still I don't get e-mail about added provider to the city.

My steps (I refreshed database and run fresh importers, just in case)

  1. Login into app.
  2. Add city to favorite (Brussels)
  3. Go to admin panel and add another provider for Brussels
  4. I expected mail, but there is not any :(

app/Events/ChangeInFavoriteCityEvent.php Outdated Show resolved Hide resolved
app/Importers/DataImporter.php Outdated Show resolved Hide resolved
app/Notifications/ChangeInFavoriteCity.php Outdated Show resolved Hide resolved
app/Notifications/ChangeInFavoriteCity.php Outdated Show resolved Hide resolved
app/Providers/EventServiceProvider.php Outdated Show resolved Hide resolved
app/Notifications/ChangeInFavoriteCity.php Outdated Show resolved Hide resolved
JakubKermes and others added 3 commits January 15, 2024 16:04
Co-authored-by: Kamil Piech <kamil.piech@blumilk.pl>
…-added-toremoved-from-favorite-city' into #90-send-e-mail-when-provider-is-added-toremoved-from-favorite-city
@JakubKermes
Copy link
Collaborator Author

@EwelinaSkrzypacz, e-mail is sent while running importers. I thought manual changes should not notify user. Should I add that functionality?

@EwelinaSkrzypacz
Copy link
Contributor

@EwelinaSkrzypacz, e-mail is sent while running importers. I thought manual changes should not notify user. Should I add that functionality?

I think that it will be nice. :)

Copy link
Contributor

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

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

After adding provider to favourite city I see 500 error

image

@JakubKermes
Copy link
Collaborator Author

@EwelinaSkrzypacz Hi, I cannot recreate your error. I get similar error message only when mailpit container is down. Could you check if you're running escooters-mailpit-dev container?

Copy link
Contributor

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

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

After clean install project I receive e-mail notification, but it looks like this

image

What's more, the notification is in English, but language of application is Polish. I have browser and OS in English, maybe that's why? I don't know, but in my opinion e-mail should be sent in language application.

@JakubKermes
Copy link
Collaborator Author

Change in appearance happened during change from email to notification, I'll fix language issue.

app/Enums/ChangeInFavouriteCityEnum.php Outdated Show resolved Hide resolved
app/Importers/DataImporter.php Outdated Show resolved Hide resolved
app/Listeners/ChangeInFavoriteCityListener.php Outdated Show resolved Hide resolved
app/Notifications/ChangeInFavoriteCity.php Outdated Show resolved Hide resolved
app/Notifications/ChangeInFavoriteCity.php Outdated Show resolved Hide resolved
app/Notifications/ChangeInFavoriteCity.php Outdated Show resolved Hide resolved
Copy link
Contributor

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

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

  1. The sentence "If you're having trouble clicking the "Dowiedz się więcej" button, copy and paste the URL below into your web browser: http://escooters.blumilk.localhost/" is not translated. And for link should redirected to the city, not to dashboard.

image

  1. The design of e-mails is still like default Laravel style. Will we update that?

lang/pl.json Outdated Show resolved Hide resolved
lang/pl.json Outdated Show resolved Hide resolved
@EwelinaSkrzypacz
Copy link
Contributor

Conflicts

@JakubKermes
Copy link
Collaborator Author

Still working on translation for the text in the bottom. I think we can leave it at default style

Copy link
Contributor

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

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

  1. E-mails works, but I think we should also translate "All right reserved"

image

  1. As I wrote above, please unify naming - in some places there is favorite and in other places favourite like ChangeInFavouriteCityEnum and ChangeInFavoriteCityEvent

lang/pl.json Outdated Show resolved Hide resolved
@kamilpiech97 kamilpiech97 changed the title #90 - send e mail when provider is added toremoved from favorite city #90 - send e mail when provider is added to removed from favorite city May 13, 2024
@EwelinaSkrzypacz EwelinaSkrzypacz changed the title #90 - send e mail when provider is added to removed from favorite city #90 - send e-mail when provider is added to or removed from favorite city May 13, 2024
@AleksandraKozubal AleksandraKozubal merged commit 7265058 into main May 13, 2024
5 checks passed
@AleksandraKozubal AleksandraKozubal deleted the #90-send-e-mail-when-provider-is-added-toremoved-from-favorite-city branch May 13, 2024 07:18
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.

Send e-mail when provider is added to/removed from favorite city
4 participants