2617: Project forecast report #175
Conversation
… in ForecastReportService
| $forecastReportData->projects[$projectId] = new ForecastReportProjectData($projectId); | ||
| } | ||
|
|
||
| if (is_object($forecastReportData->projects[$projectId])) { |
There was a problem hiding this comment.
Primarily because psalm i worried that $forecastReportData->projects[$projectId] is possible null. I rewrote it to check if it is an instance of ForecastReportProjectData, to make it feel a bit more cohesive.
|
|
||
| // Get version details from issue | ||
| $issueVersions = $worklog->getIssue()->getVersions(); | ||
| $issueVersion = count($issueVersions) > 0 ? implode('', array_map(function ($version) { return $version->getName(); }, $issueVersions->toArray())) : '[no version]'; |
There was a problem hiding this comment.
shouldn't the versions string be separated with ', ' instead of '' ?
There was a problem hiding this comment.
You are right. With Leantime as the data source, issues can only contain a single version, so i didn't think any of it.
I changed this.
src/Form/ForecastReportType.php
Outdated
|
|
||
| public function buildForm(FormBuilderInterface $builder, array $options): void | ||
| { | ||
| $dataProviders = $this->dataProviderRepository->findAll(); |
There was a problem hiding this comment.
Why do we need data providers?
Shouldn't we just show data from all or is there a need for this filter?
There was a problem hiding this comment.
It doesn't make sense to keep. I removed it.
src/Repository/WorklogRepository.php
Outdated
| $from = new \DateTimeImmutable($periodStart->format('Y-m-d').' 00:00:00'); | ||
| $to = new \DateTimeImmutable($periodEnd->format('Y-m-d').' 23:59:59'); | ||
|
|
||
| return $this->createQueryBuilder('worklog') |
There was a problem hiding this comment.
What happens if you have 1000 invoices and 1000000 worklogs?
This should be paginated and processed in pages.
Otherwise, there will be memory issues.
There was a problem hiding this comment.
There was a problem hiding this comment.
Probably something bad. I implemented this.
…e in WorklogRepository
…WorklogsAttachedToInvoiceInDateRange
| // Get worklog details | ||
| $worklogId = $worklog->getId(); | ||
| $workerEmail = $worklog->getWorker(); | ||
| $worker = $this->workerRepository->findOneBy(['email' => $workerEmail]); |
There was a problem hiding this comment.
Extract alle worker email -> name mappings out of the loops to avoid unnecessary db lookups.
Co-authored-by: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com>
src/Repository/WorklogRepository.php
Outdated
| $totalItemCount = count($paginator); | ||
| $pagesCount = ceil($totalItemCount / $pageSize); | ||
|
|
||
| $items = []; |
There was a problem hiding this comment.
why not just return the paginator instead of this conversion to array? This results in unnecessary iterations.
return [
'total_count' => $totalItemCount,
'pages_count' => $pagesCount,
'current_page' => $page,
'page_size' => $pageSize,
'paginator' => $paginator,
];
you can iterate it the same way as an array
foreach ($invoiceAttachedWorklogs['paginator'] as $worklog) {
| public function getForecastReport(\DateTimeInterface $fromDate, \DateTimeInterface $toDate): ForecastReportData | ||
| { | ||
| $page = 1; | ||
| $pageSize = 200; |
There was a problem hiding this comment.
Move to class constant
private const int PAGE_SIZE = 200;
Link to ticket
2617
Description
A report showing invoiced versus recorded hours for projects specified
Screenshot of the result
N/A
Checklist