stats4sd / aec_portfolio

A proof of concept for the AEC Consortium Project Management / Assessment System
GNU General Public License v3.0
0 stars 0 forks source link

Fix Dashboard Unexpected Message #261

Closed dan-tang-ssd closed 6 months ago

dan-tang-ssd commented 6 months ago

This PR is submitted to fix an uinexpected message in dashboard.

Root cause identified: Incorrect column used in query. In whereIn() clause, the column should be "project_id" instead of "id". The fix has been applied to both $allAssessmentsYours and $allAssessmentsOthers.

$allAssessmentsYours = $allAssessments->whereIn(
            'project_id',
            DB::table('dashboard_project')
                ->select('project_id')
                ->where('dashboard_id', $dashboardYoursId)
                ->get()
                ->pluck('project_id')
                ->toArray()
        );

Screen shots

BEFORE:

image

AFTER:

image

what-the-diff[bot] commented 6 months ago

PR Summary

These changes lead to a code that is not only easier to read but also is more robust as it is now compatible with systems using traditional syntax.

dan-tang-ssd commented 6 months ago

I have a little question about getting all assessments. For a project with re-assessment. e.g.

  1. First assessment with Complete status for redline and principle assessment
  2. Second assessment with Not Started status for redline and principle assessment

Both assessments are included, I am not sure whether it is the expected result.


Um... different perspective for including / excluding the first assessment:

  1. First assessment is a historical assessment, it happened so we should also include it.
  2. When there is a re-assessment, that means there is something changed in this project. We should include the latest assessment and exclude all previous assessments.
        // add overall score from PHP side because this is already calculated on the Assessment Model.
        $allAssessments = Project::withoutGlobalScope('organisation')->with([
            'assessments' => ['principleAssessments.principle', 'principles', 'failingRedlines']
        ])
            // only initiatives that are fully assessed
            ->whereHas('assessments', function (Builder $query) {
                $query->where('redline_status', AssessmentStatus::Failed->value)
                    ->orWhere('principle_status', AssessmentStatus::Complete->value);
            })
            ->get()
            ->pluck('assessments')
            ->flatten();
dave-mills commented 6 months ago

Both assessments are included, I am not sure whether it is the expected result.

Um... different perspective for including / excluding the first assessment:

Mm, good point. You're right this is something worth reviewing. The overall goal is to look at the current state of the organisation's portfolios. If a project gets re-assessed, the latest assessment is the one to include. Older ones are kept in the database for accounting and review purposes, but I think they should be skipped in the analysis.

Quick check in the generate_dashboard_summary - this is how it works there:

    -- find latest assessments of projects (others)
    INSERT INTO dashboard_assessment (dashboard_id, assessment_id)
    SELECT dashboardOthersId, MAX(id) AS latest_assessment_id
    FROM assessments
    WHERE project_id IN
          (SELECT project_id FROM dashboard_project where dashboard_id = dashboardOthersId)
    GROUP BY project_id;

So you're right - we should update the logic for getting $allAssessments to match.

dan-tang-ssd commented 6 months ago

In dashboard, I have revised program to exclude previously completed assessments.

I tried to modify the eloquent query for $allAssessments, but failed.

For clarity and readability, I added a separate query to obtain all latest assessments. And then use it on collection to keep latest assessments only.