Skip to content

chore: GET /students multiple status filter #764

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.

Sign up for GitHub

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

Open
kirisaki-vk wants to merge 2 commits into
base: preprod
Choose a base branch
from
Open

chore: GET /students multiple status filter #764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b101e44
chore: GET /students multiple status filters specs
kirisaki-vk Dec 18, 2024
caf2ac3
chore: GET /students multiple status filters
kirisaki-vk Dec 18, 2024
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
4 changes: 3 additions & 1 deletion doc/api.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,9 @@ paths:
in: query
description: Filter students by status value return all by default
schema:
$ref: '#/components/schemas/EnableStatus'
type: array
items:
$ref: '#/components/schemas/EnableStatus'

@haritianaadriano haritianaadriano Jan 30, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it's time to rename this variable. And use a better name like UserStatus or StudentStatus, EnableStatus means only ENABLE as status no ?

@Mahefaa Mahefaa Feb 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agreed, however it will be a breaking change

- name: sex
required: false
in: query
Expand Down
6 changes: 4 additions & 2 deletions src/main/java/school/hei/haapi/endpoint/rest/controller/StudentController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,15 @@ public List<Student> getStudents(
@RequestParam(value = "first_name", required = false, defaultValue = "") String firstName,
@RequestParam(value = "last_name", required = false, defaultValue = "") String lastName,
@RequestParam(value = "course_id", required = false, defaultValue = "") String courseId,
@RequestParam(name = "status", required = false) EnableStatus status,
@RequestParam(name = "status", required = false, defaultValue = "ENABLED,SUSPENDED")
List<EnableStatus> status,

@Mahefaa Mahefaa Feb 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

userStatuses

@Mahefaa Mahefaa Feb 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

param name can be the same for retro-compatibility however you MUST change your variable names

@RequestParam(name = "sex", required = false) Sex sex,
@RequestParam(name = "work_study_status", required = false) WorkStudyStatus workStatus,
@RequestParam(name = "commitment_begin_date", required = false) Instant commitmentBeginDate,
@RequestParam(name = "exclude_groups", required = false) List<String> excludeGroupIds) {
User.Sex domainSex = sexEnumMapper.toDomainSexEnum(sex);
User.Status domainStatus = statusEnumMapper.toDomainStatus(status);
List<User.Status> domainStatus =
status == null ? null : status.stream().map(statusEnumMapper::toDomainStatus).toList();
Comment on lines +87 to +88

@haritianaadriano haritianaadriano Jan 30, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

handle it in the mapper method, and why you give a default value as ENABLED, SUSPENDED but you handle nullable value here ?

@Mahefaa Mahefaa Feb 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agreed, also list variables should be named with plural nouns, hence statuses
2) a list should never be null but empty or populated.

return userService
.getByLinkedCourse(
STUDENT,
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/school/hei/haapi/repository/dao/UserManagerDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public List<User> findByCriteria(
String firstName,
String lastName,
Pageable pageable,
User.Status status,
List<User.Status> status,

@Mahefaa Mahefaa Feb 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

userStatuses, static import User.Status

User.Sex sex,
WorkStudyStatus workStatus,
Instant commitmentBeginDate,
Expand Down Expand Up @@ -80,7 +80,7 @@ public List<User> findByCriteria(
}

if (status != null) {
predicate = builder.and(predicate, builder.equal(root.get("status"), status));
predicate = builder.and(predicate, root.get("status").in(status));
}

if (sex != null) {
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/school/hei/haapi/service/UserService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,10 @@ public List<User> getByCriteria(
User.Sex sex) {
Pageable pageable =
PageRequest.of(page.getValue() - 1, pageSize.getValue(), Sort.by(ASC, "ref"));
List<User.Status> statusList = status == null ? null : List.of(status);

@haritianaadriano haritianaadriano Jan 30, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why you handle always the null value. But ok, i'll be fine with it but remember sometimes it's a bad things to use null as value instead it's better to use Optional or List and check if it's empty if a List and check if present if Optional

@Mahefaa Mahefaa Jan 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if status null, List.of(), else List.of(status) a List object should never be null


return userManagerDao.findByCriteria(
role, ref, firstName, lastName, pageable, status, sex, null, null, null, null, null);
role, ref, firstName, lastName, pageable, statusList, sex, null, null, null, null, null);
}

public List<User> getByLinkedCourse(
Expand All @@ -227,7 +228,7 @@ public List<User> getByLinkedCourse(
String courseId,
PageFromOne page,
BoundedPageSize pageSize,
User.Status status,
List<User.Status> status,

@Mahefaa Mahefaa Feb 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

userStatuses, static import User.Status

User.Sex sex,
WorkStudyStatus workStatus,
Instant commitmentBeginDate,
Expand Down
61 changes: 50 additions & 11 deletions src/test/java/school/hei/haapi/integration/StudentIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static school.hei.haapi.endpoint.rest.model.EnableStatus.DISABLED;
import static school.hei.haapi.endpoint.rest.model.EnableStatus.ENABLED;
import static school.hei.haapi.endpoint.rest.model.EnableStatus.SUSPENDED;
import static school.hei.haapi.endpoint.rest.model.PaymentFrequency.MONTHLY;
Expand Down Expand Up @@ -79,7 +80,6 @@
import school.hei.haapi.endpoint.rest.client.ApiException;
import school.hei.haapi.endpoint.rest.model.Coordinates;
import school.hei.haapi.endpoint.rest.model.CrupdateStudent;
import school.hei.haapi.endpoint.rest.model.EnableStatus;
import school.hei.haapi.endpoint.rest.model.Fee;
import school.hei.haapi.endpoint.rest.model.Group;
import school.hei.haapi.endpoint.rest.model.Statistics;
Expand Down Expand Up @@ -271,7 +271,7 @@ public static Student disabledStudent1() {
.lastName("One")
.email("test+disable1@hei.school")
.ref("STD29001")
.status(EnableStatus.DISABLED)
.status(DISABLED)
.sex(M)
.birthDate(LocalDate.parse("2000-12-01"))
.entranceDatetime(Instant.parse("2021-11-08T08:25:24.00Z"))
Expand Down Expand Up @@ -498,8 +498,7 @@ void manager_read_by_disabled_status_ok() throws ApiException {
UsersApi api = new UsersApi(manager1Client);

List<Student> actualStudents =
api.getStudents(
1, 10, null, null, null, null, EnableStatus.DISABLED, null, null, null, null);
api.getStudents(1, 10, null, null, null, null, List.of(DISABLED), null, null, null, null);
assertEquals(2, actualStudents.size());
assertTrue(actualStudents.contains(disabledStudent1()));
}
Expand All @@ -511,7 +510,7 @@ void manager_read_by_suspended_status_ok() throws ApiException {
UsersApi api = new UsersApi(manager1Client);

List<Student> actualStudents =
api.getStudents(1, 10, null, null, null, null, SUSPENDED, null, null, null, null);
api.getStudents(1, 10, null, null, null, null, List.of(SUSPENDED), null, null, null, null);
assertEquals(1, actualStudents.size());
assertTrue(actualStudents.contains(suspendedStudent1()));
}
Expand All @@ -535,7 +534,7 @@ void manager_read_by_status_and_sex_ok() throws ApiException {
UsersApi api = new UsersApi(manager1Client);

List<Student> actualStudents =
api.getStudents(1, 10, null, null, null, null, EnableStatus.DISABLED, F, null, null, null);
api.getStudents(1, 10, null, null, null, null, List.of(DISABLED), F, null, null, null);
assertEquals(1, actualStudents.size());
}

Expand Down Expand Up @@ -988,7 +987,8 @@ void manager_write_suspended_student() throws ApiException {
List<Student> actual = api.createOrUpdateStudents(List.of(creatableSuspendedStudent()), null);
Student created = actual.get(0);
List<Student> suspended =
api.getStudents(1, 10, null, "Suspended", null, null, SUSPENDED, null, null, null, null);
api.getStudents(
1, 10, null, "Suspended", null, null, List.of(SUSPENDED), null, null, null, null);

assertTrue(suspended.contains(created));
assertEquals(1, actual.size());
Expand All @@ -1004,7 +1004,7 @@ void manager_update_student_to_suspended() throws ApiException {
api.createOrUpdateStudents(List.of(createStudent2().status(SUSPENDED)), null);
Student updated = actual.getFirst();
List<Student> suspended =
api.getStudents(1, 10, null, null, null, null, SUSPENDED, null, null, null, null);
api.getStudents(1, 10, null, null, null, null, List.of(SUSPENDED), null, null, null, null);

assertTrue(suspended.contains(updated));
assertEquals(1, actual.size());
Expand All @@ -1016,11 +1016,50 @@ void stats_are_exact() throws ApiException {
UsersApi usersApi = new UsersApi(manager1Client);

Integer women =
usersApi.getStudents(1, 200, null, null, null, null, null, F, null, null, null).size();
usersApi
.getStudents(
1,
200,
null,
null,
null,
null,
List.of(SUSPENDED, ENABLED, DISABLED),
F,
null,
null,
null)
.size();
Integer men =
usersApi.getStudents(1, 200, null, null, null, null, null, M, null, null, null).size();
usersApi
.getStudents(
1,
200,
null,
null,
null,
null,
List.of(SUSPENDED, ENABLED, DISABLED),
M,
null,
null,
null)
.size();
Integer totalStudents =
usersApi.getStudents(1, 200, null, null, null, null, null, null, null, null, null).size();
usersApi
.getStudents(
1,
200,
null,
null,
null,
null,
List.of(SUSPENDED, ENABLED, DISABLED),

@haritianaadriano haritianaadriano Jan 30, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

and where you check Student with only two status ?

null,
null,
null,
null)
.size();

Statistics statistics = usersApi.getStats();
assertEquals(statistics.getWomen().getTotal(), women);
Expand Down