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

fix(module:notification): don't create new messageId for update #8000

Merged

Conversation

Nicoss54
Copy link
Collaborator

@Nicoss54 Nicoss54 commented Jun 30, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

This PR is a fix for generating a new messageId for updating notification

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

When we update a notification, the service generate a new messageId

Issue Number: 7975

What is the new behavior?

When we update a notification, the service don't generate a new messageId

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@Nicoss54 Nicoss54 requested a review from cipchk as a code owner June 30, 2023 15:18
@zorro-bot
Copy link

zorro-bot bot commented Jun 30, 2023

This preview will be available after the AzureCI is passed.

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #8000 (a5820ba) into master (605e969) will increase coverage by 91.70%.
The diff coverage is 100.00%.

❗ Current head a5820ba differs from pull request most recent head 6e77b95. Consider uploading reports for the commit 6e77b95 to get more accurate results

@@             Coverage Diff             @@
##           master    #8000       +/-   ##
===========================================
+ Coverage        0   91.70%   +91.70%     
===========================================
  Files           0      514      +514     
  Lines           0    17574    +17574     
  Branches        0     2690     +2690     
===========================================
+ Hits            0    16117    +16117     
- Misses          0     1162     +1162     
- Partials        0      295      +295     
Impacted Files Coverage Δ
components/message/base.ts 93.44% <100.00%> (ø)
components/notification/notification.service.ts 100.00% <100.00%> (ø)

... and 512 files with indirect coverage changes

@Nicoss54
Copy link
Collaborator Author

Nicoss54 commented Jul 1, 2023

@simplejason could you check why the Pipeline is not trigger ? still in the same status Waiting for status to be reported

@Nicoss54 Nicoss54 force-pushed the fix/same-instance-notification-update branch from ad6eb2f to 6e77b95 Compare July 1, 2023 11:11
@Nicoss54 Nicoss54 closed this Jul 1, 2023
@Nicoss54 Nicoss54 reopened this Jul 1, 2023
@Nicoss54
Copy link
Collaborator Author

Nicoss54 commented Jul 1, 2023

@simplejason could you check why the Pipeline is not trigger ? still in the same status Waiting for status to be reported

It’s fix ;)

@Nicoss54 Nicoss54 force-pushed the fix/same-instance-notification-update branch from 6e77b95 to 8c555d0 Compare July 11, 2023 06:35
Copy link
Member

@simplejason simplejason left a comment

Choose a reason for hiding this comment

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

LGTM

@simplejason simplejason merged commit e240264 into NG-ZORRO:master Jul 16, 2023
7 checks passed
@Nicoss54 Nicoss54 deleted the fix/same-instance-notification-update branch August 18, 2023 14:45
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.

messageId changing when updating notification through nzKey
2 participants