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 adding user properties to UserInfo context #243

Merged
merged 5 commits into from
Jun 25, 2024

Conversation

Arciiix
Copy link
Contributor

@Arciiix Arciiix commented Jun 25, 2024

I have noticed another bug related to user preferences. #242 that I pushed yesterday fixes saving those user preferences to the database, but they are still not loaded into the UserInfo context, therefore they are not accessible from any of the messaging integrations.

To reproduce, you can again set up the Telegram integration and add a breakpoint here:

public void AddTargets(IDictionary<string, string> targets, UserInfo user)
{
var chatId = GetChatId(user);
if (!string.IsNullOrWhiteSpace(chatId))
{
targets[TelegramChatId] = chatId;
}
}

No matter what the user preferences are, the chatId will always be empty (because it doesn't exist in the Properties of UserInfo instance).

My fix merges the SystemProperties field together with the UserProperties, prioritizing the SystemProperties (therefore the .Last()) - because they were what was previously being set there - to ensure this fix doesn't break anything.

All the tests related to this are passing.

PS: I'm finishing the Discord integration. I'm doing those small pull requests that are general fixes not related to the integration itself, so that those changes are atomic (as you suggested in #38 ).

Have a nice day!

Id = user.Id,
EmailAddress = user.EmailAddress,
PhoneNumber = user.PhoneNumber,
Properties = user.Properties
Copy link
Collaborator

@SebastianStehle SebastianStehle Jun 25, 2024

Choose a reason for hiding this comment

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

I am not a big fan of LINQ in this case. Can we just make it with a normal dictionary like

var properties = new Dictionary<string, string>(user.Properties);

if (user.SystemProperties != null)
{
   foreach (var (key, value) in user.SystemProperties)
   {
      properties[key] = values;
   } 
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, of course. Pushing the new changes...

Copy link
Collaborator

@SebastianStehle SebastianStehle left a comment

Choose a reason for hiding this comment

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

See my comment

@SebastianStehle
Copy link
Collaborator

Btw: Notifo test coverage is really bad. Feel free to add new tests

@SebastianStehle SebastianStehle merged commit 573c097 into notifo-io:main Jun 25, 2024
2 checks passed
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.

None yet

2 participants