jessHuh
commented
1 year ago Closed weihengSu2 closed 1 year ago
jessHuh
commented
1 year ago 
weihengSu2
commented
1 year ago Could you add some tests? https://github.com/Azure/azure-notificationhubs-dotnet/blob/main/src/Microsoft.Azure.NotificationHubs.Tests/NotificationHubClientTest.cs
Hey Jessica yes initially I want to add. but we need to call PROD-BL2-001 and send a xiaomi notification or registration or installation there. Then put all of their data like urls / content type and etc into MockData folder. Currently we haven't enabled the UseNewPipelineForXiaomi feature flag on PROD-BL2-001 neither we have supported the xiaomi installation or registration in Azure Notification hub repo (v1/v2).
Once we support them and enable the flag on prod, I will immediately add back the CRUD operations as well as the unit tests in NotificationHubClientTests. I have them already locally
jessHuh
commented
1 year ago Could you add some tests? https://github.com/Azure/azure-notificationhubs-dotnet/blob/main/src/Microsoft.Azure.NotificationHubs.Tests/NotificationHubClientTest.cs
Hey Jessica yes initially I want to add. but we need to call PROD-BL2-001 and send a xiaomi notification or registration or installation there. Then put all of their data like urls / content type and etc into MockData folder. Currently we haven't enabled the UseNewPipelineForXiaomi feature flag on PROD-BL2-001 neither we have supported the xiaomi installation or registration in Azure Notification hub repo (v1/v2).
Once we support them and enable the flag on prod, I will immediately add back the CRUD operations as well as the unit tests in NotificationHubClientTests. I have them already locally
Interesting. Do we want to merge this PR before we enable the feature flag? We don't want to merge half working Xiaomi registration CRUD operations, do we?
weihengSu2
commented
1 year ago Could you add some tests? https://github.com/Azure/azure-notificationhubs-dotnet/blob/main/src/Microsoft.Azure.NotificationHubs.Tests/NotificationHubClientTest.cs
Hey Jessica yes initially I want to add. but we need to call PROD-BL2-001 and send a xiaomi notification or registration or installation there. Then put all of their data like urls / content type and etc into MockData folder. Currently we haven't enabled the UseNewPipelineForXiaomi feature flag on PROD-BL2-001 neither we have supported the xiaomi installation or registration in Azure Notification hub repo (v1/v2). Once we support them and enable the flag on prod, I will immediately add back the CRUD operations as well as the unit tests in NotificationHubClientTests. I have them already locally
Interesting. Do we want to merge this PR before we enable the feature flag? We don't want to merge half working Xiaomi registration CRUD operations, do we?
It is okay to merge it. The flag is enabled in INT7 and another testing ScaleUnit. We need to let the partner team test it. For this PR, it is only registration related classes and there is not any CRUD operation in this PR
jessHuh
commented
1 year ago Okay but why do we need to call BL2? If it's enabled in INT, could we write a test pointing to INT7?
weihengSu2
commented
1 year ago Okay but why do we need to call BL2? If it's enabled in INT, could we write a test pointing to INT7?
we could point it to INT7 but it is like in MockData folder and NotificationHubClientTest.cs, we use a namespace sdk-nh-sample, and it is on PROD-BL2-001. It is actually okay to use INT7 but it is just not the same as other test data. But if you guys would like, I could always do it on INT7 for Xiaomi notification but for registration and installation, I bet that we need to wait for Azure Notification Hub (v1 / v2) to complete the code so we could store Xiaomi Installation / Registration into the database 🙂
Things to consider before you submit the PR:
Description
This PR will add Xiaomi registration description class and template registration description class. The PR will also add Xiaomi registration crud operations