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

Move directory #162

Merged
merged 27 commits into from
Oct 25, 2024
Merged

Move directory #162

merged 27 commits into from
Oct 25, 2024

Conversation

YenguiSeddik
Copy link
Contributor

No description provided.

Seddik Yengui added 2 commits September 13, 2024 11:15
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
@YenguiSeddik YenguiSeddik changed the title Move directory [WIP]Move directory Sep 13, 2024
@souissimai souissimai self-requested a review September 24, 2024 15:00
Seddik Yengui added 2 commits September 25, 2024 00:05
fix
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
@YenguiSeddik YenguiSeddik changed the title [WIP]Move directory Move directory Sep 26, 2024
Seddik Yengui and others added 2 commits September 26, 2024 16:15
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
@jamal-khey jamal-khey self-requested a review September 30, 2024 09:26
@jamal-khey jamal-khey self-requested a review September 30, 2024 22:28
notifyDirectoryHasChanged(oldDirectory.getId(), userId, element.getName());

}

private void validateElementForMove(DirectoryElementEntity element, UUID newDirectoryUuid, String userId) {
if (element.getType().equals(DIRECTORY)) {
throw new DirectoryException(IS_DIRECTORY);
if (Objects.equals(element.getType(), DIRECTORY) && element.getParentId() == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

element.getType can be null? why using Objects.equals

@@ -586,9 +586,29 @@ public void testMoveDirectory() throws Exception {
.header("userId", "Doe")
.contentType(MediaType.APPLICATION_JSON)
.content(objectMapper.writeValueAsString(List.of(directory21UUID))))
.andExpect(status().isForbidden());
.andExpect(status().isOk());
Copy link
Contributor

Choose a reason for hiding this comment

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

add rootdirectory case? forbidden

Seddik Yengui added 11 commits October 2, 2024 14:30
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
fix
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
fix
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
fix
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
@SlimaneAmar SlimaneAmar self-requested a review October 11, 2024 11:00
@SlimaneAmar
Copy link
Contributor

Just need a test to move a root directory

Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
@SlimaneAmar
Copy link
Contributor

Remove Sonar issue !

Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
Copy link
Contributor

@jamal-khey jamal-khey left a comment

Choose a reason for hiding this comment

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

The tests and code look good overall. However, there is an issue with moving the directory to the same location, as we still receive a notification on the front end.

Comment on lines 335 to 337
if (element.getParentId() == newDirectoryUuid) { // Same directory ?
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

small bug here , '==' does not work on uuids

Suggested change
if (element.getParentId() == newDirectoryUuid) { // Same directory ?
return;
}
if (element.getParentId().equals(newDirectoryUuid)) { // Same directory ?
return;
}

may be we need a small unit test for this case also ?

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, i used Objects.equals(element.getParentId(), newDirectoryUuid) because parentId could be null

Signed-off-by: Seddik Yengui <seddik.yengui@rte-france.com>
Copy link

@YenguiSeddik YenguiSeddik merged commit d98a00a into main Oct 25, 2024
3 checks passed
@YenguiSeddik YenguiSeddik deleted the move_directory branch October 25, 2024 14:55
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.

4 participants