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

Delete multiple elements #76

Merged
merged 7 commits into from
Feb 26, 2024
Merged

Delete multiple elements #76

merged 7 commits into from
Feb 26, 2024

Conversation

YenguiSeddik
Copy link
Contributor

No description provided.

@Meklo Meklo self-requested a review February 15, 2024 18:51
@sBouzols sBouzols requested a review from achour94 February 20, 2024 09:10
@YenguiSeddik YenguiSeddik requested a review from Meklo February 20, 2024 13:31
@@ -242,6 +242,16 @@ public ResponseEntity<Void> deleteElement(@PathVariable("elementUuid") UUID elem
return ResponseEntity.ok().build();
}

@DeleteMapping(value = "/explore/elements/{directoryUuid}/delete-stashed")
@Operation(summary = "Remove directories/elements")
@ApiResponses(value = {@ApiResponse(responseCode = "200", description = "Filter creation request delegated to filter server")})
Copy link
Contributor

Choose a reason for hiding this comment

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

That is not the right description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed it

@ApiResponses(value = {@ApiResponse(responseCode = "200", description = "Filter creation request delegated to filter server")})
public ResponseEntity<Void> deleteElements(@RequestParam("ids") List<UUID> elementsUuid,
@RequestHeader("userId") String userId,
@PathVariable String directoryUuid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable is not used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but we must have an uuid at this position or the gateway will reject our fetch with 403 status

// FIXME dirty fix to ignore errors and still delete the elements in the directory-server. To delete when handled properly.
} catch (Exception e) {
LOGGER.error(e.toString(), e);
directoryService.deleteDirectoryElements(uuids, userId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this method is called in both try and catch !?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the same logic of this PR: #27. we ignore the exception and do the deletion anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

consider this suggestion
public void deleteElements(List uuids, String userId) {
try {
uuids.forEach(id -> directoryService.deleteElement(id, userId));
// FIXME dirty fix to ignore errors and still delete the elements in the directory-server. To delete when handled properly.
} catch (Exception e) {
LOGGER.error(e.toString(), e);
} finally {
directoryService.deleteDirectoryElements(uuids, userId);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link

@YenguiSeddik YenguiSeddik merged commit 5e625cf into main Feb 26, 2024
3 checks passed
@YenguiSeddik YenguiSeddik deleted the delete-multiple_elements branch February 26, 2024 08:25
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