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

Add a way to specify username to send notifications to #16073

Merged
merged 13 commits into from
May 17, 2024
Merged

Add a way to specify username to send notifications to #16073

merged 13 commits into from
May 17, 2024

Conversation

MikeAlhayek
Copy link
Member

Fix #14748

@MikeAlhayek MikeAlhayek changed the title Add a way to specify username to notify Add a way to specify username to send notifications to May 16, 2024
Piedone
Piedone requested changes May 16, 2024
View reviewed changes
src/OrchardCore.Modules/OrchardCore.Notifications/Activities/NotifyUserTask.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.Notifications/Activities/NotifyUserTask.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.Notifications/ViewModels/NotifyUserTaskViewModel.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.Notifications/Views/Items/NotifyUserTask.Fields.Edit.cshtml Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.Notifications/Views/Items/NotifyUserTask.Fields.Edit.cshtml Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.Notifications/Drivers/NotifyUserTaskDisplayDriver.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.Notifications/Drivers/NotifyUserTaskDisplayDriver.cs Outdated Show resolved Hide resolved
@MikeAlhayek MikeAlhayek requested a review from Piedone May 16, 2024 18:58
@Piedone
Copy link
Member

Piedone commented May 16, 2024

Please only request review if we have all the discussions settled. I really don't want to keep getting back to things.

@MikeAlhayek
Copy link
Member Author

@Piedone I made the requested changes. I would rather keep Username instead of UserName. Let me know if you have any additional changes.

@MikeAlhayek
Copy link
Member Author

@Piedone I am done with this. I made some changes to simplify inheriting the base display driver. Let me know if you see anything else. Otherwise, please approve. I would like to test this out using "preview" in my project tomorrow.

Piedone
Piedone requested changes May 16, 2024
View reviewed changes
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

You can BTW dotnet pack just this project locally with a higher version number, reference it from your web project with a local package source, and test it right away.

src/OrchardCore.Modules/OrchardCore.Notifications/Drivers/NotifyUserTaskDisplayDriver.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.Notifications/Drivers/NotifyUserTaskDisplayDriver.cs Outdated Show resolved Hide resolved
@MikeAlhayek @Piedone
Update src/OrchardCore.Modules/OrchardCore.Notifications/Drivers/Noti…
228caa4 
…fyUserTaskDisplayDriver.cs

Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
@MikeAlhayek MikeAlhayek requested a review from Piedone May 16, 2024 22:42
@MikeAlhayek
Copy link
Member Author

You can BTW dotnet pack just this project locally with a higher version number, reference it from your web project with a local package source, and test it right away.

@Piedone this would require you to pack 4 different packages since more than one project was updated. I will wait until this is merged since everything was addressed anyway and the PR is ready.

@Piedone
Copy link
Member

Piedone commented May 16, 2024

Not everything: #16073 (comment)

@Piedone Piedone merged commit 0f57494 into main May 17, 2024
16 checks passed
@Piedone Piedone deleted the ma/add-username-toNotifyUserTask branch May 17, 2024 00:20
This was referenced Jun 1, 2024
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Piedone Piedone Piedone 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.

Remove useless NotifyUserTask
2 participants
@MikeAlhayek @Piedone