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

Updated clan route for clan opening #343

Closed
wants to merge 5 commits into from
Closed
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
27 changes: 18 additions & 9 deletions src/main/java/com/faforever/api/clan/ClanService.java
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@ Clan create(String name, String tag, String description, Player creator) {
clanRepository.save(clan);
return clan;
}

@SneakyThrows
String generatePlayerInvitationToken(Player requester, int clanId) {
// Playerless invitation
Copy link
Member

Choose a reason for hiding this comment

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

Either java doc or remove. Or maybe generatePlayerInvitationTokenWithoutPlayer

return generatePlayerInvitationToken(requester, 0, int clanId);
Copy link

Choose a reason for hiding this comment

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

You can't have int clanId

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, oops
but i dont even know if overloading is the right thing to do here

Copy link
Member

Choose a reason for hiding this comment

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

Why can u not? And yeah overloading is fine

}

@SneakyThrows
String generatePlayerInvitationToken(Player requester, int newMemberId, int clanId) {
Expand All @@ -93,12 +99,14 @@ String generatePlayerInvitationToken(Player requester, int newMemberId, int clan
throw new ApiException(new Error(ErrorCode.CLAN_NOT_LEADER, clanId));
}

Player newMember = playerRepository.findById(newMemberId)
.orElseThrow(() -> new ApiException(new Error(ErrorCode.CLAN_GENERATE_LINK_PLAYER_NOT_FOUND, newMemberId)));
Player newMember = clan.getIsOpen() ? null :
Copy link
Member

Choose a reason for hiding this comment

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

What should happen if the clan is not open.?For me it seems like there should not be an invite link generated, if so please throw a custom ApiException

Copy link
Member

Choose a reason for hiding this comment

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

I mean if newMember if null and expire is 0 that seems weird to me.

playerRepository.findById(newMemberId)
.orElseThrow(() -> new ApiException(new Error(ErrorCode.CLAN_GENERATE_LINK_PLAYER_NOT_FOUND, newMemberId)));

long expire = Instant.now()
.plus(fafApiProperties.getClan().getInviteLinkExpireDurationMinutes(), ChronoUnit.MINUTES)
.toEpochMilli();
long expire = clan.getIsOpen() ? 0 :
Instant.now()
.plus(fafApiProperties.getClan().getInviteLinkExpireDurationMinutes(), ChronoUnit.MINUTES)
.toEpochMilli();

InvitationResult result = new InvitationResult(expire,
ClanResult.of(clan),
Expand All @@ -120,11 +128,12 @@ void acceptPlayerInvitationToken(String stringToken, Authentication authenticati
Clan clan = clanRepository.findById(clanId)
.orElseThrow(() -> new ApiException(new Error(ErrorCode.CLAN_NOT_EXISTS, clanId)));

Player newMember = playerRepository.findById(invitation.getNewMember().getId())
.orElseThrow(() -> new ProgrammingError("ClanMember does not exist: " + invitation.getNewMember().getId()));
Player newMember = clan.getIsOpen() ? null :
playerRepository.findById(invitation.getNewMember().getId())
.orElseThrow(() -> new ProgrammingError("ClanMember does not exist: " + invitation.getNewMember().getId()));
Copy link
Member

Choose a reason for hiding this comment

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

Throw an ApiException always those are the ones with custom error codes and text to them



if (player.getId() != newMember.getId()) {
if (!invitation.getClan().getIsOpen() && player.getId() != newMember.getId()) {
Copy link
Member

Choose a reason for hiding this comment

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

Throw a different custom ApiException with your own error here. Otherwise test will be missleading

throw new ApiException(new Error(ErrorCode.CLAN_ACCEPT_WRONG_PLAYER));
}
if (newMember.getClan() != null) {
Expand All @@ -133,7 +142,7 @@ void acceptPlayerInvitationToken(String stringToken, Authentication authenticati

ClanMembership membership = new ClanMembership();
membership.setClan(clan);
membership.setPlayer(newMember);
membership.setPlayer(player);
clanMembershipRepository.save(membership);
}
}
8 changes: 8 additions & 0 deletions src/main/java/com/faforever/api/data/domain/Clan.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public class Clan extends AbstractEntity implements OwnableEntity {
private String description;
private String tagColor;
private String websiteUrl;
private boolean isOpen;
private List<ClanMembership> memberships;

@Column(name = "name")
Expand Down Expand Up @@ -107,4 +108,11 @@ public String getWebsiteUrl() {
public Login getEntityOwner() {
return getLeader();
}

@Column(name = "isOpen")
@NotNull
@UpdatePermission(expression = IsEntityOwner.EXPRESSION)
public bool getIsOpen() {
return isOpen;
}
}
50 changes: 50 additions & 0 deletions src/test/java/com/faforever/api/clan/ClanServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ public void createClanSuccessful() {
assertEquals(creator, clanCaptor.getValue().getFounder());
assertEquals(1, clanCaptor.getValue().getMemberships().size());
assertEquals(creator, clanCaptor.getValue().getMemberships().get(0).getPlayer());
assertEquals(false, clanCaptor.getValue().getIsOpen());
}

@Test
Expand Down Expand Up @@ -231,6 +232,35 @@ public void generatePlayerInvitationToken() throws IOException {
assertEquals(clan.getName(), captor.getValue().getClan().getName());
}

@Test
public void generatePlayerOpenInvitationToken() throws IOException {
Player requester = new Player();
requester.setId(1);

Clan clan = ClanFactory.builder().leader(requester).build().setIsOpen(false);

FafApiProperties props = new FafApiProperties();

when(clanRepository.findById(clan.getId())).thenReturn(Optional.of(clan));
when(fafApiProperties.getClan()).thenReturn(props.getClan());

instance.generatePlayerInvitationToken(requester, clan.getId());

// What do i do with this??
// Invitation result should be modified to accept null players but i have no IDE here so navigating is a pain
Copy link
Member

Choose a reason for hiding this comment

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

Get an IDE and remove this comment

/*
ArgumentCaptor<InvitationResult> captor = ArgumentCaptor.forClass(InvitationResult.class);
verify(jwtService, Mockito.times(1)).sign(captor.capture());
assertEquals(captor.getValue().getExpire(), 0);
assertEquals(null, captor.getValue().getNewMember());
*/
assertEquals(clan.getId(), getValue().getClan().getId());
/*
assertEquals(clan.getTag(), captor.getValue().getClan().getTag());
assertEquals(clan.getName(), captor.getValue().getClan().getName());
*/
}

@Test
public void acceptPlayerInvitationTokenExpire() throws IOException {
String stringToken = "1234";
Expand All @@ -249,6 +279,26 @@ public void acceptPlayerInvitationTokenExpire() throws IOException {
}
verify(clanMembershipRepository, Mockito.never()).save(any(ClanMembership.class));
}

@Test
public void acceptPlayerOpenInvitationCannotExpire() throws IOException {
// Why is the clan passed nowhere here?? How do I make it not expire if the clan is open since no clan is passed as part of the test?
Copy link
Member

Choose a reason for hiding this comment

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

This is all stored in the token

String stringToken = "1234";
long expire = System.currentTimeMillis();
Jwt jwtToken = Mockito.mock(Jwt.class);

when(jwtToken.getClaims()).thenReturn(
String.format("{\"expire\":%s}", expire));
when(jwtService.decodeAndVerify(any())).thenReturn(jwtToken);

try {
instance.acceptPlayerInvitationToken(stringToken, null);
fail();
} catch (ApiException e) {
assertThat(e, hasErrorCode(ErrorCode.CLAN_ACCEPT_TOKEN_EXPIRE));
}
verify(clanMembershipRepository, Mockito.never()).save(any(ClanMembership.class));
}

@Test
public void acceptPlayerInvitationTokenInvalidClan() throws IOException {
Expand Down