Skip to content
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

New inspections view #1880

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Conversation

mrica-equinor
Copy link
Contributor

@mrica-equinor mrica-equinor commented Dec 13, 2024

Ready for review checklist:

  • A self-review has been performed
  • All commits run individually
  • Temporary changes have been removed, like console.log, TODO, etc.
  • The PR has been tested locally
  • A test have been written
    • This change doesn't need a new test
  • Relevant issues are linked
  • Remaining work is documented in issues
    • There is no remaining work from this PR that require new issues
  • The changes does not introduce dead code as unused imports, functions etc.

@mrica-equinor mrica-equinor force-pushed the new-inspections-view branch 2 times, most recently from 25b9326 to a63fa6f Compare December 13, 2024 11:22
@mrica-equinor
Copy link
Contributor Author

@mrica-equinor mrica-equinor self-assigned this Dec 13, 2024
@mrica-equinor mrica-equinor added feature New feature or request backend Backend related functionality frontend Frontend related functionality Northern Lights labels Dec 13, 2024
Comment on lines +74 to +79
try
{
inspection = await inspectionService.ReadByIsarTaskId(taskId, readOnly: true);
if (inspection == null) return NotFound($"Could not find inspection for task with Id {taskId}.");

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider querying the inspection by the inspectionID instead of the taskID


if (!inspectionData.BlobContainer.ToLower(CultureInfo.CurrentCulture).Equals(installationCode.ToLower(CultureInfo.CurrentCulture), StringComparison.Ordinal))
{
return NotFound($"Could not find inspection data for inspection with Id {inspection.Id} because blob name {inspectionData.BlobName} does not match installation {installationCode}.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving the checks into the InspectionService

@@ -430,4 +430,12 @@ export class BackendAPICaller {
})
return result.content
}

static async getInspection(installationCode: string, taskId: string): Promise<Blob> {
const path: string = 'inspection/' + installationCode + '/' + taskId + '/taskId'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider querying by inspectionId instead of taskId

Comment on lines +69 to +71
.catch(() => {
setStartTimer((oldValue) => !oldValue)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider stop trying if we get a failed workflow from the backend

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added issue: #1881

<Typography key={task.id + task.inspection.id + 'insp'} link href={task.inspection.inspectionUrl}>
{TranslateText(task.inspection.inspectionType as string)}
</Typography>
(task.inspection.inspectionType === InspectionType.Image ? (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future we can also do this for ThermalImage

Copy link
Contributor

@tsundvoll tsundvoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! Looking forward to see it in the frontend. Some comments, but we can solve these in future PRs

@mrica-equinor mrica-equinor merged commit be7d191 into equinor:main Dec 13, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Backend related functionality feature New feature or request frontend Frontend related functionality Northern Lights
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants