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 updating the user preferences #242

Merged
merged 1 commit into from
Jun 24, 2024
Merged

Conversation

Arciiix
Copy link
Contributor

@Arciiix Arciiix commented Jun 24, 2024

As mentioned in #38 , updating the user properties didn't work previously.

In the user upsert function,

public override async ValueTask<User?> ExecuteAsync(User target, IServiceProvider serviceProvider,

The properties are meant to be kept in the Properties field

if (Is.Changed(Properties, target.Properties))
{
newUser = newUser with
{
Properties = Properties
};
}

While on the front-end side, the properties are sent directly at the "root" level of the DTO, e.g. here's an example POST request

{
  "requests": [
    {
      "id": "c1729fd5-527d-497f-97c5-db31b69af57d",
      ...
      "properties": {}, // Empty object, should contain the properties
      "settings": {},
      "counters": {},
      "mobilePushTokens": [],
      "webPushSubscriptions": [],
      "userProperties": [
        {
          "name": "telegramChatId",
          "editorDescription": "The ID to identity the user in the chat with the bot. Will be set automatically.",
          "editorLabel": "Telegram Chat ID"
        },
        {
          "name": "telegramUserId",
          "editorDescription": "The username in telegram.",
          "editorLabel": "Telegram Username"
        }
      ],
        // Those properties shouldn't be here!
        "telegramChatId": "chat id", 
        "telegramUserId": "user id"
    }
  ]
}

That's because the form properties weren't registered inside "properties", but rather at the root level of the DTO.

On the other hand, the DTO on front-end is correct and assumes the properties are kept in the properties field

/** The user properties. */
properties?: { [key: string]: string; } | undefined;

This pull request contains the fixes for this.

@SebastianStehle
Copy link
Collaborator

SebastianStehle commented Jun 24, 2024

Could you update the tests here: https://github.com/notifo-io/notifo/blob/main/tools/TestSuite/TestSuite.ApiTests/UsersTests.cs

I know that your fix is on the UI, but just to be sure.

@Arciiix
Copy link
Contributor Author

Arciiix commented Jun 24, 2024

Do you mean creating additional tests for that Properties field? I ran all the tests before pushing the code and everything (besides the integration tests that need API keys etc.) is fine

@SebastianStehle
Copy link
Collaborator

Yes, but thats an integration tests. To run them you have to configure the backend:

  1. Create and appSettings.Development.json file

  2. Add the following content

{
    "identity": {
       // Client with all admin permissions.
       "adminClientId": "root",
       "adminClientSecret": "xeLd6jFxqbXJrfmNLlO2j1apagGGGSyZJhFnIuHp4I0=",
       ```
     }
}

This will create a special client for authentication that is used by the integraiton tests.

@Arciiix
Copy link
Contributor Author

Arciiix commented Jun 24, 2024

Sure, thank you for your reply. I'll run those tests later.
So about this pull request itself, I think it's ready to merge, or do you want me to do something more related to this?

@SebastianStehle SebastianStehle merged commit fe94068 into notifo-io:main Jun 24, 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.

2 participants