-
Notifications
You must be signed in to change notification settings - Fork 55
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
separated 'admins-and-groupings' into 2 endpoints 'groupings-admins' & 'all-groupings' #511
separated 'admins-and-groupings' into 2 endpoints 'groupings-admins' & 'all-groupings' #511
Conversation
Overall, great work! Also make sure to update your code style file in IntelliJ and fixup the import order in all the Java files. You can send me or Isaac a message if you have any questions. I will review your UI PR once the changes have been made to this PR because you are likely to have to make changes to the UI side too. |
39a2c6e
to
8ffd0f3
Compare
src/test/java/edu/hawaii/its/api/groupings/GroupingAllTest.java
Outdated
Show resolved
Hide resolved
src/test/java/edu/hawaii/its/api/groupings/GroupingAllTest.java
Outdated
Show resolved
Hide resolved
Final thing, make sure the functions and classes created/touched in this PR have 100% test coverage or as close as to 100% as you can. Here is how to check the test coverage percentage: https://uhawaii.atlassian.net/wiki/spaces/SITARd/pages/12943618/jaCoCo+Code+Coverage |
247c598
to
8490dd7
Compare
src/main/java/edu/hawaii/its/api/service/GroupingAssignmentService.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/hawaii/its/api/service/GroupingAssignmentService.java
Outdated
Show resolved
Hide resolved
public GroupingAdmins groupingAdmins (String adminUhIdentifier) { | ||
logger.info(String.format("groupingAdmins; adminUhIdentifier: %s;", adminUhIdentifier)); | ||
if (!memberService.isAdmin(adminUhIdentifier)) { | ||
throw new AccessDeniedException(); | ||
} | ||
AdminListsHolder adminListsHolder = new AdminListsHolder(); | ||
List<String> adminGrouping = Arrays.asList(GROUPING_ADMINS); | ||
Group admin = getMembers(adminUhIdentifier, adminGrouping).get(GROUPING_ADMINS); | ||
adminListsHolder.setAllGroupingPaths(groupingsService.allGroupingPaths()); | ||
adminListsHolder.setAdminGroup(admin); | ||
return adminListsHolder; | ||
Group admin = this.getMembers(adminUhIdentifier, Arrays.asList(GROUPING_ADMINS)).get(GROUPING_ADMINS); | ||
return new GroupingAdmins(admin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a better idea for this function, let's change up the GroupingAdmins
to store a GroupingGroupMembers
(Note the "s" only after Members) instead of a Group
. That would mean that grouperApiService.getMembersResult()
should be called in this function. Also notice the constructor for GroupingGroupMembers
takes in GetMembersResult
. Making this change will match the code with our current API design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also could you remove the space before (string adminUhIdentifier)
on line 62? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package edu.hawaii.its.api.groupings;
import edu.hawaii.its.api.wrapper.GetMembersResult;
import edu.hawaii.its.api.wrapper.Subject;
import java.util.List;
public class GroupingAdmins {
private GroupingGroupMembers adminGroupingGroupMembers;
public GroupingAdmins() {
this.adminGroupingGroupMembers = new GroupingGroupMembers();
this.adminGroupingGroupMembers.setResultCode("FAILURE");
}
public GroupingAdmins(GetMembersResult getMembersResult) {
this.adminGroupingGroupMembers = new GroupingGroupMembers(getMembersResult);
this.adminGroupingGroupMembers.setResultCode("SUCCESS");
}
public List<GroupingGroupMember> getAdminGroupMembers() { return adminGroupingGroupMembers.getMembers(); }
public void setAdminGroupMembers(List<Subject> adminGroup) { this.adminGroupingGroupMembers.setMembers(adminGroup); }
public String getResultCode() { return this.adminGroupingGroupMembers.getResultCode(); }
}
Does it look right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it have a 3rd constructor taking List<Subjects>
as a parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused about how this should be implemented and what i need to return in the getters and set in the setters. Is it List<Subject>
or GroupingGroupMembers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GroupingAdmins should continue to implement GroupingResult
. The adminGroupingGroupMembers
can just be admins
for short.
You only need a 3rd constructor if you will use it.
The getter should be of type GroupingGroupMembers
. The setter in this case is not needed and should be used as private a function to help with your constructors. You can see the other classes in the /groupings folder that do it like that.
Sorry for the confusion, I took a deeper look into the |
22c3f66
to
b57a9b0
Compare
94b7fef
to
5770b0e
Compare
5770b0e
to
de0b6fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Ticket Link
Ticket
List of squashed commits
Test Checklist