Skip to content

Conversation

@sachin-maheshwari
Copy link

…eploying topic name change.

@vikasrohit vikasrohit requested a review from maxceem August 14, 2019 10:18
@sachin-maheshwari sachin-maheshwari merged commit cdad37f into dev Aug 14, 2019
Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

It looks like this fix wouldn't work as expected. I know the logic here is quite complex and has many details. In this place, we are matching notificationRule with notification if their type is the same. But in provided solution notificationRule would be matched to notification if notification has old prefix and it would not take into account the actual notification type. So all the next notification types would be treated equal (the first value is rule type and the second value is notification type. As a result, notificationRule would be selected quite randomly.

image

I guess the best way to fix it client-side is to update notification types when we get data from the server. We can hardcode updating old names to new names by kind of transforming old pattern to new pattern as it was done for updating notification settings in the DB https://github.com/topcoder-platform/tc-notifications/blob/def92f2935b0ba445fe8d12e46c3909dd7b401f7/migrations/v2.0.1.sql

Though it feels that efforts to rename it in DB are quite same as to patch it in Connect. So it would be better to do it in the DB of the Tc-notifications service using SQL script. As notifications already have quite complex logic with versions and rules we can do that once in the DB to avoid intorducing a new variation clients-side.

@vikasrohit
Copy link

Thanks @maxceem for the detailed review.

@sachin-maheshwari
Copy link
Author

sachin-maheshwari commented Aug 14, 2019

@maxceem FYI, warnings you shared are coming before merging as well.

@maxceem
Copy link
Collaborator

maxceem commented Aug 17, 2019

Right @sachin-maheshwari. We have warnings to show that some notifications have types which we don't know how to process in Connect. After the notification types have been renamed in configs but not in DB we have many warnings. Because types in configs were updated, but for existent notifications in DB we have old types.

After the fix in this PR, most of warnings disappear because after this fix by new logic we already can find rules for notifications from the DB, but they are not correct.

So I've added new warnings in the screenshot above. They show which notifications rules we associate to which notifications types. So it can be seen, that we associate notification rules with type which don't match notification types.

@vikasrohit
Copy link

Agreed with @maxceem Reverting the PR.

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.

4 participants