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

[MODLISTS-57] Soft deletion within lists #57

Merged
merged 9 commits into from
Jan 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions src/main/java/org/folio/list/controller/ListController.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
package org.folio.list.controller;

import java.time.OffsetDateTime;
import java.time.format.DateTimeFormatter;
import java.util.List;
import java.util.UUID;
import lombok.RequiredArgsConstructor;
import org.folio.list.domain.dto.ListDTO;
import org.folio.list.domain.dto.ListRefreshDTO;
Expand All @@ -19,11 +23,6 @@
import org.springframework.util.StringUtils;
import org.springframework.web.bind.annotation.RestController;

import java.time.OffsetDateTime;
import java.time.format.DateTimeFormatter;
import java.util.List;
import java.util.UUID;

@RequiredArgsConstructor
@RestController
public class ListController implements ListApi {
Expand All @@ -34,16 +33,28 @@ public class ListController implements ListApi {
public ResponseEntity<ListSummaryResultsDTO> getAllLists(List<UUID> ids,
List<UUID> entityTypeIds,
Integer offset,
Integer size, Boolean active,
Integer size,
Boolean active,
Boolean isPrivate, // Note: query param name is "private"
Boolean includeDeleted,
String updatedAsOf
) {
OffsetDateTime providedTimestamp;
DateTimeFormatter formatter = DateTimeFormatter.ISO_OFFSET_DATE_TIME;
// In the backend, the plus sign (+) that is received through RequestParams within the provided timestamp gets substituted with a blank space.
providedTimestamp = !StringUtils.hasText(updatedAsOf) ? null : OffsetDateTime.parse(updatedAsOf.replace(' ', '+'), formatter);
Pageable pageable = new OffsetRequest(offset, size);
return ResponseEntity.ok(listService.getAllLists(pageable, ids, entityTypeIds, active, isPrivate, providedTimestamp));
return ResponseEntity.ok(
listService.getAllLists(
pageable,
ids,
entityTypeIds,
active,
isPrivate,
Boolean.TRUE.equals(includeDeleted),
providedTimestamp
)
);
}

@Override
Expand Down
8 changes: 7 additions & 1 deletion src/main/java/org/folio/list/domain/ListEntity.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@
import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.NoArgsConstructor;
import lombok.With;

import org.folio.list.domain.dto.ListUpdateRequestDTO;
import org.folio.list.exception.AbstractListException;
import org.folio.list.rest.UsersClient.User;
import org.folio.list.util.TaskTimer;

@Data
@With
Copy link
Member Author

Choose a reason for hiding this comment

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

This generates methods like withIsDeleted, withName, etc., which clones and edits one field in a fluent manner.

Copy link
Collaborator

@mweaver-ebsco mweaver-ebsco Jan 24, 2024

Choose a reason for hiding this comment

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

Totally out of scope and I'm not asking for any changes, but I just need to complain...

ngl, I really hate everything about this class, but I don't care enough to go out of my way to change it... It's like a perfect storm of things that annoy me: odd class name + lombok + ORM + mutability + leaky abstraction (it has a dependency on something in UserClient) + business logic in a data class (with no comments, no less)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. We could do a quick clean up noise-wise at least by removing the @Column's (it'll derive from fields), and changing some to primitives (removes @NotNull's) — super minor change, but would save a smidge of sanity

@Entity
@AllArgsConstructor
@NoArgsConstructor
Expand Down Expand Up @@ -101,12 +104,15 @@ public class ListEntity {
private ListRefreshDetails failedRefresh;

@Column(name = "version")
@NotNull
private int version;

@Column(name = "user_friendly_query")
private String userFriendlyQuery;

@Column(name = "is_deleted")
@NotNull
private Boolean isDeleted;

public boolean isRefreshing() {
return inProgressRefresh != null;
}
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/folio/list/mapper/ListEntityMapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ public interface ListEntityMapper {
@Mapping(target = "updatedBy", expression = "java(createdBy.id())")
@Mapping(target = "updatedByUsername", expression = "java(createdBy.getFullName().orElse(createdBy.id().toString()))")
@Mapping(target = "isCanned", constant = "false")
@Mapping(target = "isDeleted", constant = "false")
@Mapping(target = "version", constant = "1")
ListEntity toListEntity(ListRequestDTO request, UsersClient.User createdBy);
}
18 changes: 16 additions & 2 deletions src/main/java/org/folio/list/repository/ListRepository.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,14 @@

import java.time.OffsetDateTime;
import java.util.List;
import java.util.Optional;
import java.util.UUID;

@Repository
public interface ListRepository extends CrudRepository<ListEntity, UUID>, PagingAndSortingRepository<ListEntity, UUID> {

Optional<ListEntity> findByIdAndIsDeletedFalse(UUID id);
Copy link
Member Author

Choose a reason for hiding this comment

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

Spring derives the query for us


@Query(
value = """
SELECT l
Expand All @@ -24,6 +27,7 @@ public interface ListRepository extends CrudRepository<ListEntity, UUID>, Paging
AND (l.isPrivate = false OR l.updatedBy = :currentUserId OR ( l.updatedBy IS NULL AND l.createdBy = :currentUserId))
AND (:isPrivate IS NULL OR l.isPrivate = :isPrivate)
AND (:active IS NULL OR l.isActive = :active)
AND (:includeDeleted = true OR l.isDeleted = false)
AND (TO_TIMESTAMP(CAST(:updatedAsOf AS text), 'YYYY-MM-DD HH24:MI:SS.MS') IS NULL OR
(l.createdDate>= TO_TIMESTAMP(CAST(:updatedAsOf AS text), 'YYYY-MM-DD HH24:MI:SS.MS') OR
l.updatedDate>= TO_TIMESTAMP(CAST(:updatedAsOf AS text), 'YYYY-MM-DD HH24:MI:SS.MS')))
Expand All @@ -37,12 +41,22 @@ SELECT count(*)
AND (l.isPrivate = false OR l.updatedBy = :currentUserId OR ( l.updatedBy IS NULL AND l.createdBy = :currentUserId))
AND (:isPrivate IS NULL OR l.isPrivate = :isPrivate)
AND (:active IS NULL OR l.isActive = :active)
AND (:includeDeleted = true OR l.isDeleted = false)
AND (TO_TIMESTAMP(CAST(:updatedAsOf AS text), 'YYYY-MM-DD HH24:MI:SS.MS') IS NULL OR
(l.createdDate>= TO_TIMESTAMP(CAST(:updatedAsOf AS text), 'YYYY-MM-DD HH24:MI:SS.MS') OR
l.updatedDate>= TO_TIMESTAMP(CAST(:updatedAsOf AS text), 'YYYY-MM-DD HH24:MI:SS.MS')))
"""
)
Page<ListEntity> searchList(Pageable pageable, List<UUID> ids, List<UUID> entityTypeIds, UUID currentUserId, Boolean active, Boolean isPrivate, OffsetDateTime updatedAsOf);
)
Page<ListEntity> searchList(
Pageable pageable,
List<UUID> ids,
List<UUID> entityTypeIds,
UUID currentUserId,
Boolean active,
Boolean isPrivate,
boolean includeDeleted,
OffsetDateTime updatedAsOf
);

}

36 changes: 23 additions & 13 deletions src/main/java/org/folio/list/services/ListService.java
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,21 @@ public class ListService {
private final ListVersionRepository listVersionRepository;
private final ListVersionMapper listVersionMapper;

public ListSummaryResultsDTO getAllLists(Pageable pageable, List<UUID> ids,
List<UUID> entityTypeIds, Boolean active, Boolean isPrivate, OffsetDateTime updatedAsOf) {
public ListSummaryResultsDTO getAllLists(Pageable pageable, List<UUID> ids, List<UUID> entityTypeIds, Boolean active,
Boolean isPrivate, boolean includeDeleted, OffsetDateTime updatedAsOf) {

log.info("Attempting to get all lists");
UUID currentUserId = executionContext.getUserId();
Page<ListEntity> lists = listRepository.searchList(pageable, isEmpty(ids) ? null : ids,
isEmpty(entityTypeIds) ? null : entityTypeIds, currentUserId, active, isPrivate, updatedAsOf);
Page<ListEntity> lists = listRepository.searchList(
pageable,
isEmpty(ids) ? null : ids,
isEmpty(entityTypeIds) ? null : entityTypeIds,
currentUserId,
active,
isPrivate,
includeDeleted,
updatedAsOf
);

// List database do not store entity type labels. Only entity type ID is available in List database.
// Get the corresponding entity type labels from FQM
Expand Down Expand Up @@ -129,7 +137,7 @@ public ListDTO createList(ListRequestDTO listRequest) {

public Optional<ListDTO> updateList(UUID id, ListUpdateRequestDTO request) {
log.info("Attempting to update a list with id : {}", id);
Optional<ListEntity> listEntity = listRepository.findById(id);
Optional<ListEntity> listEntity = listRepository.findByIdAndIsDeletedFalse(id);
listEntity.ifPresent(list -> {
EntityType entityType = getEntityType(list.getEntityTypeId());
validationService.validateUpdate(list, request, entityType);
Expand Down Expand Up @@ -165,7 +173,7 @@ public Optional<ListDTO> updateList(UUID id, ListUpdateRequestDTO request) {

public Optional<ListDTO> getListById(UUID id) {
log.info("Attempting to get specific list for id {}", id);
return listRepository.findById(id)
return listRepository.findByIdAndIsDeletedFalse(id)
.map(list -> {
validationService.assertSharedOrOwnedByUser(list, ListActions.READ);
return listMapper.toListDTO(list);
Expand All @@ -174,7 +182,7 @@ public Optional<ListDTO> getListById(UUID id) {

public Optional<ListRefreshDTO> performRefresh(UUID listId) {
log.info("Attempting to refresh list with listId {}", listId);
return listRepository.findById(listId)
return listRepository.findByIdAndIsDeletedFalse(listId)
.map(list -> {
validationService.validateRefresh(list);
list.refreshStarted(getCurrentUser());
Expand All @@ -201,23 +209,23 @@ public Optional<ListRefreshDTO> performRefresh(UUID listId) {
public Optional<ResultsetPage> getListContents(UUID listId, Integer offset, Integer size) {
log.info("Attempting to get contents for list with listId {}, tenantId {}, offset {}, size {}",
listId, executionContext.getTenantId(), offset, size);
return listRepository.findById(listId)
return listRepository.findByIdAndIsDeletedFalse(listId)
.map(list -> {
validationService.assertSharedOrOwnedByUser(list, ListActions.READ);
return getListContents(list, offset, size);
});
}

public void deleteList(UUID id) {
ListEntity list = listRepository.findById(id)
ListEntity list = listRepository.findByIdAndIsDeletedFalse(id)
.orElseThrow(() -> new ListNotFoundException(id, ListActions.DELETE));
validationService.validateDelete(list);
deleteListAndContents(list);
}

public void cancelRefresh(UUID listId) {
log.info("Cancelling refresh for list {}", listId);
ListEntity list = listRepository.findById(listId)
ListEntity list = listRepository.findByIdAndIsDeletedFalse(listId)
.orElseThrow(() -> new ListNotFoundException(listId, ListActions.CANCEL_REFRESH));
validationService.validateCancelRefresh(list);
list.refreshCancelled(executionContext.getUserId());
Expand All @@ -227,7 +235,7 @@ public void cancelRefresh(UUID listId) {
public List<ListVersionDTO> getListVersions(UUID listId) {
log.info("Checking that list {} is accessible and exists before getting versions", listId);

ListEntity list = listRepository.findById(listId).orElseThrow(() -> new ListNotFoundException(listId, ListActions.READ));
ListEntity list = listRepository.findByIdAndIsDeletedFalse(listId).orElseThrow(() -> new ListNotFoundException(listId, ListActions.READ));
validationService.assertSharedOrOwnedByUser(list, ListActions.READ);

log.info("Getting all versions of the list {}", listId);
Expand All @@ -243,7 +251,9 @@ public List<ListVersionDTO> getListVersions(UUID listId) {
public ListVersionDTO getListVersion(UUID listId, int version) {
log.info("Checking that list {} is accessible and exists before getting version {}", listId, version);

ListEntity list = listRepository.findById(listId).orElseThrow(() -> new ListNotFoundException(listId, ListActions.READ));
ListEntity list = listRepository
.findByIdAndIsDeletedFalse(listId)
.orElseThrow(() -> new ListNotFoundException(listId, ListActions.READ));
validationService.assertSharedOrOwnedByUser(list, ListActions.READ);

log.info("Getting version {} of the list {}", version, listId);
Expand All @@ -256,7 +266,7 @@ public ListVersionDTO getListVersion(UUID listId, int version) {

private void deleteListAndContents(ListEntity list) {
listContentsRepository.deleteContents(list.getId());
listRepository.deleteById(list.getId());
listRepository.save(list.withIsDeleted(true));
}

private ResultsetPage getListContents(ListEntity list, Integer offset, Integer limit) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public class ListExportService {

@Transactional
public ListExportDTO createExport(UUID listId) {
ListEntity list = listRepository.findById(listId)
ListEntity list = listRepository.findByIdAndIsDeletedFalse(listId)
.orElseThrow(() -> new ListNotFoundException(listId, ListActions.EXPORT));
validationService.validateCreateExport(list);
ExportDetails exportDetails = createExportDetails(list);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import org.springframework.transaction.annotation.Transactional;

import java.util.UUID;
import java.util.function.BiConsumer;
import java.util.function.Predicate;

@Service
Expand Down Expand Up @@ -51,7 +50,7 @@ private void saveFailedRefresh(ListEntity entity, TaskTimer timer, Throwable fai
* inProgressRefreshId for this list in database.
*/
private boolean isActiveRefresh(UUID listId, UUID refreshId) {
return listRepository.findById(listId)
return listRepository.findByIdAndIsDeletedFalse(listId)
.flatMap(ListEntity::getInProgressRefreshId)
.filter(Predicate.isEqual(refreshId))
.isPresent();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ private void saveSuccessRefresh(ListEntity entity, Integer recordsCount, TaskTim
* inProgressRefreshId for this list in database.
*/
private boolean isActiveRefresh(UUID listId, UUID refreshId) {
return listRepository.findById(listId)
return listRepository.findByIdAndIsDeletedFalse(listId)
.flatMap(ListEntity::getInProgressRefreshId)
.filter(Predicate.isEqual(refreshId))
.isPresent();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@
<include file="yml/update-list-versions-fkey.yaml" relativeToChangelogFile="true"/>
<include file="yml/alter-list-details-updated-metadata-add-not-null-constraint.yaml" relativeToChangelogFile="true"/>
<include file="sql/update-list-refresh-details-table.sql" relativeToChangelogFile="true"/>
<include file="yml/add-list-details-is-deleted-column.yaml" relativeToChangelogFile="true"/>
</databaseChangeLog>
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
databaseChangeLog:
- changeSet:
id: add-list-details-is-deleted-column
author: [email protected]
changes:
- addColumn:
tableName: list_details
columns:
- column:
name: is_deleted
type: boolean
defaultValueBoolean: false
constraints:
nullable: false
6 changes: 6 additions & 0 deletions src/main/resources/swagger.api/list.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ paths:
required: false
schema:
type: boolean
- name: includeDeleted
in: query
description: Indicates if deleted lists should be included in the results (default false)
required: false
schema:
type: boolean
- name: updatedAsOf
in: query
description: Indicates the minimum create/update timestamp to filter lists by
Expand Down
4 changes: 4 additions & 0 deletions src/main/resources/swagger.api/schemas/ListDTO.json
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@
"description": "Indicates if a List is canned or not",
"type": "boolean"
},
"isDeleted" : {
"description": "Indicates if a List has been deleted",
"type": "boolean"
},
"updatedBy": {
"description": "ID of the user who last updated the record (when available)",
"type": "string",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@
"description": "Indicates if a List is canned or not",
"type": "boolean"
},
"isDeleted" : {
"description": "Indicates if a List has been deleted",
"type": "boolean"
},
"updatedBy": {
"description": "ID of the user who last updated the record (when available)",
"type": "string",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

import static org.hamcrest.Matchers.is;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.ArgumentMatchers.isNull;
import static org.mockito.Mockito.when;
import static org.springframework.http.MediaType.APPLICATION_JSON;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
Expand Down Expand Up @@ -54,8 +56,8 @@ void testGetAllLists() throws Exception {
.contentType(APPLICATION_JSON)
.header(XOkapiHeaders.TENANT, TENANT_ID);

when(listService.getAllLists(any(Pageable.class), Mockito.eq(null),
Mockito.eq(null), Mockito.eq(null), Mockito.eq(null), Mockito.eq(null))).thenReturn(listSummaryResultsDto);
when(listService.getAllLists(any(Pageable.class), isNull(), isNull(),
isNull(), isNull(), eq(false), isNull())).thenReturn(listSummaryResultsDto);

mockMvc.perform(requestBuilder)
.andExpect(status().isOk())
Expand Down Expand Up @@ -87,11 +89,12 @@ void testGetAllListsWithIdsParameter() throws Exception {
.queryParam("entityTypeIds", listDto1.getEntityTypeId().toString(), listDto2.getEntityTypeId().toString())
.queryParam("active", "true")
.queryParam("private", "true")
.queryParam("includeDeleted", "false")
.queryParam("updatedAsOf", "2023-01-27T20:54:41.528281+05:30");


when(listService.getAllLists(any(Pageable.class), Mockito.eq(listIds),
Mockito.eq(listEntityIds), Mockito.eq(true), Mockito.eq(true), Mockito.eq(providedTimestamp)))
Mockito.eq(listEntityIds), Mockito.eq(true), Mockito.eq(true), Mockito.eq(false), Mockito.eq(providedTimestamp)))
.thenReturn(listSummaryResultsDto);

mockMvc.perform(requestBuilder)
Expand Down Expand Up @@ -127,7 +130,7 @@ void testGetAllListsWithVariableOffsetAndSize() throws Exception {
.queryParam("private", "false");

when(listService.getAllLists(pageable, null,
null, false, false, null)).thenReturn(listSummaryResultsDto);
null, false, false, false, null)).thenReturn(listSummaryResultsDto);

mockMvc.perform(requestBuilder)
.andExpect(status().isOk())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,6 @@ void shouldMapDtoToEntity() {
assertEquals(user.id(), listEntity.getCreatedBy());
assertNotNull(listEntity.getCreatedDate());
assertFalse(listEntity.getIsCanned());
assertFalse(listEntity.getIsDeleted());
}
}
Loading