Heedls 459 usage stats report #443
Conversation
| WHERE (LogYear = @year AND LogMonth IN @months AND CentreID = @centreId) | ||
| GROUP BY LogYear, LogMonth | ||
| SELECT @year AS Year, m.Month, COALESCE(a.Completions, 0) AS Completions, COALESCE(a.Evaluations, 0) AS Evaluations, COALESCE(a.Registrations, 0) AS Registrations |
There was a problem hiding this comment.
Can we break down this long line a little
DigitalLearningSolutions.Web/Controllers/TrackingSystem/Centre/Reports/ReportsController.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/ViewModels/TrackingSystem/Centre/Reports/ReportsViewModel.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Reports/_ActivityTable.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Reports/Index.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Reports/_ActivityTable.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Reports/_ActivityTable.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Reports/_ActivityTable.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Reports/Index.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Reports/Index.cshtml
Show resolved
Hide resolved
DigitalLearningSolutions.Web/ViewModels/TrackingSystem/Centre/Reports/ReportsViewModel.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/ViewModels/TrackingSystem/Centre/Reports/ReportsViewModel.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Controllers/TrackingSystem/Centre/Reports/ReportsController.cs
Outdated
Show resolved
Hide resolved
| this.connection = connection; | ||
| } | ||
|
|
||
| public IEnumerable<MonthOfActivity> GetActivityForMonthsInYear(int centreId, int year, IEnumerable<int> months) |
There was a problem hiding this comment.
I think this method should be getting monthly activity between two dates, but group the data by LogYear & LogMonth in SQL as you've done now. I digged out the stored procedure used for the old site (uspGetRegComp) and it looks like it just filters on LogDate and then uses LogYear & LogMonth for easy grouping.
| WHERE (LogDate > @start AND LogDate < @end AND CentreID = @centreId) | ||
| GROUP BY LogYear, LogMonth | ||
| ORDER BY LogYear, LogMonth", | ||
| new {centreId, start, end} |
There was a problem hiding this comment.
Minor, but I think this need a quick formatter :)
| using FluentAssertions; | ||
| using NUnit.Framework; | ||
|
|
||
| class ActivityDataServiceTests |
There was a problem hiding this comment.
We usually declare the test classes as public + the ActivityServiceTests
| return months; | ||
| } | ||
|
|
||
| private IEnumerable<MonthOfActivity> GenerateBlankMonthsBetweenDates(DateTime startDate, DateTime endDate) |
There was a problem hiding this comment.
I think this is great, but I'd probably return a IEnumerable<(int year, int month)> from this method and pull it into a helper, where we can write very specific unit tests for this and reuse it in the future.
We could also do something like this above (a simple map to the object we need):
var monthSlots = DateTimeHelper.GenerateMonthSlots(..);
return monthSlots.Select(slot => {
var monthlyUsageStats = ...
return new MonthOfActivity(slot, monthlyUsageStats ) // Need to add a constructor
});
| var monthEnumerable = Enumerable.Range(startDate.Month, diffInMonths + 1); | ||
|
|
||
| return monthEnumerable.Select( | ||
| m => ((m - 1) % 12 + 1, startDate.Year + (m - 1) / 12) |
There was a problem hiding this comment.
Minor, but it might be nice to pull these into var month and var yearsToAdd, e.g.
m => {
var month = (m - 1) % 12 + 1;
var yearsToAdd = (m - 1) / 12;
return (month, startDate.Year + yearsToAdd);
}
3 participants
I think it's probably the case that the reports want to be in an expandable, 12 months is a lot.