-
Notifications
You must be signed in to change notification settings - Fork 320
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 Scheduler Entry Form To get PlanID from Plan Repo #791
Fix Scheduler Entry Form To get PlanID from Plan Repo #791
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
src/AdminSite/Controllers/SchedulerController.cs:235
- Add a null check for 'subscriptionDetail' to prevent potential NullReferenceException.
var subscriptionDetail = this.subscriptionService.GetActiveSubscriptionsWithMeteredPlan().Where(s => s.Id == Convert.ToInt32(schedulerUsageViewModel.SelectedSubscription)).FirstOrDefault();
src/AdminSite/Controllers/SchedulerController.cs:237
- Add a null check for 'selectedPlan' to prevent potential NullReferenceException.
var selectedPlan = this.plansRepository.GetById(subscriptionDetail.AmpplanId);
src/AdminSite/Controllers/SchedulerController.cs:234
- [nitpick] Improve the comment to be more descriptive, e.g., 'Retrieve the active subscription detail to get the AMP Plan ID'.
//Get AMP Plan ID from Subscription Detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
src/AdminSite/Controllers/SchedulerController.cs:239
- [nitpick] The error message could be more descriptive by including the subscriptionId to aid in debugging.
this.logger.LogError("Subscription detail not found for the given subscription ID.");
src/AdminSite/Controllers/SchedulerController.cs:247
- [nitpick] The error message could be more descriptive by including the AmpplanId to aid in debugging.
this.logger.LogError("Plan detail not found for the given subscription.");
src/AdminSite/Controllers/SchedulerController.cs:243
- Ensure that the new behavior of retrieving the plan by AmpplanId is covered by tests.
var selectedPlan = this.plansRepository.GetById(subscriptionDetail.AmpplanId);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good
Pass: Fix Scheduler Entry Form To get PlanID from Plan Repo
return this.View("Error", "Subscription detail not found."); | ||
} | ||
// Retrieve the active Plan detail by AMP Plan ID | ||
var selectedPlan = this.plansRepository.GetById(subscriptionDetail.AmpplanId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msalemcode are we able to get the selected plan by ampplanId and offer id please? Imagine a scenarios a different planid with same name exists for both offers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
retested and approved with changes to Services/Services
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added quick comment
Update Add New Schedule code to get PlanID from Plan Repo instead of Using Dimension to look up plan Id