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

Feature/416 get report data from import #982

Merged
merged 11 commits into from
Feb 15, 2024

Conversation

ds-lcapellino
Copy link

No description provided.

Copy link

github-actions bot commented Feb 14, 2024

Integration Test Results

322 tests   322 ✅  1m 9s ⏱️
 37 suites    0 💤
 37 files      0 ❌

Results for commit 55b137b.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Feb 14, 2024

Unit Test Results

215 tests   215 ✅  20s ⏱️
 54 suites    0 💤
 54 files      0 ❌

Results for commit 55b137b.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Feb 14, 2024

✅ No Dependency Check findings were found

@ds-ashanmugavel ds-ashanmugavel marked this pull request as ready for review February 14, 2024 16:03
Copy link

@ds-ashanmugavel ds-ashanmugavel left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@ds-ext-sceronik ds-ext-sceronik left a comment

Choose a reason for hiding this comment

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

Open for conversation regarding comments :)

Map<AssetBase, Boolean> resultMap = null;
try {
resultMap = importService.importAssets(file);
resultMap = importService.importAssets(file, importJob);

Choose a reason for hiding this comment

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

should result map be returned in this case ? if there is job handling then I would guess service would return importJobId only . and FE would get report from that ID rather than from result map in here ?

} catch (ImportException e) {
log.info("Could not import data", e);
importService.cancelJob(importJob);

Choose a reason for hiding this comment

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

same here . if there is import job that should be updated with error ( what happened ) why return error response right away ? if we want to migrate to job approach this endpoint should return either validation error for invalid format like it is doing now or job ID . then at later time FE would call job report to get report on what actually happened

private UUID id;
private Instant startedOn;
private Instant completedOn;
private ImportJobStatus status;

Choose a reason for hiding this comment

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

if we go with jobID returned to FE we should consider also persisting information which entities/assets were not persisted and why. otherways user would not know what exactly happened with created job and he would only see successfully persisted entities

Copy link

@ds-ext-sceronik ds-ext-sceronik left a comment

Choose a reason for hiding this comment

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

After talking and considering this is as described in concept Approved ;)

Copy link

Copy link

Quality Gate Passed Quality Gate passed for 'TX Traceability FOSS frontend'

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

@ds-ext-sceronik ds-ext-sceronik left a comment

Choose a reason for hiding this comment

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

lgtm

@ds-lcapellino ds-lcapellino merged commit 756723c into main Feb 15, 2024
@ds-lcapellino ds-lcapellino deleted the feature/416-get-report-data-from-import branch February 15, 2024 15:00
ds-mwesener added a commit that referenced this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants