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

Updated clan route for clan opening #343

wants to merge 5 commits into from

Conversation

Rackover
Copy link
Member

Dont look at me like I know what I'm doing please

Disable player and expiration checks if the clan is marked as "open"
@Rackover
Copy link
Member Author

Related to FAForever/db#206

@SneakyThrows
String generatePlayerInvitationToken(Player requester, int clanId) {
// Playerless invitation
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 clanId) {
// Playerless invitation
return generatePlayerInvitationToken(requester, 0, int clanId);
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 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

@@ -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.



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

.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

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


@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

@Rackover
Copy link
Member Author

Rackover commented Oct 2, 2019

This is less a real PR than a desperate cry for help from a java developer who is more experienced than me

@1-alex98
Copy link
Member

1-alex98 commented Oct 2, 2019

sorry did not read that out of the comment u wrote i thought u meant to do a real PR

@1-alex98
Copy link
Member

1-alex98 commented Oct 2, 2019

I would have said lets do discord+ team viewer and do it together but not sure if u are the biggest fan of that

@Brutus5000
Copy link
Member

Brutus5000 commented Nov 10, 2019

I do not agree on the general approach here. Generating fake invite tokens are not a good approach to solve this issue.
I will work out a cleaner approach in #362

@Brutus5000 Brutus5000 closed this Nov 10, 2019
@Brutus5000 Brutus5000 deleted the is_clan_open branch January 24, 2020 22:05
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