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.

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

Jump to bottom

Ban Revoke structure #281

Merged
merged 1 commit into from
Dec 14, 2018
Merged

Ban Revoke structure #281

merged 1 commit into from
Dec 14, 2018

Conversation

1-alex98
Copy link
Member

@1-alex98 1-alex98 commented Nov 1, 2018

Fixes #280

@1-alex98
Copy link
Member Author

1-alex98 commented Nov 1, 2018

The correct faf-stack version does not exist yet

@1-alex98 1-alex98 force-pushed the feature/#280-ban-revoke-structure branch 7 times, most recently from a06a1d2 to 81e8a54 Compare November 1, 2018 22:44
Brutus5000
Brutus5000 requested changes Nov 4, 2018
View reviewed changes
Copy link
Member

@Brutus5000 Brutus5000 left a comment

Choose a reason for hiding this comment

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

Please add integration tests to BanInfoTest.java
They were missing before, because it did not work with Elide. Now as it should be working, we should have tests.

src/main/java/com/faforever/api/data/domain/BanInfo.java Outdated
@JoinColumn(name = "author_id")
@NotNull
Copy link
Member

Choose a reason for hiding this comment

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

Why was @NotNull removed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was removed because it is set now anyway so no @NotNull nedded

Copy link
Member

Choose a reason for hiding this comment

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

No. Implementations may change, but there is nothing more explicit than validation right on the entity. No reason to remove it. It gives additional information and enforces a validation.

@1-alex98 1-alex98 force-pushed the feature/#280-ban-revoke-structure branch 2 times, most recently from 4927c36 to 92252f9 Compare November 11, 2018 12:15
@1-alex98 1-alex98 force-pushed the feature/#280-ban-revoke-structure branch from 92252f9 to a480525 Compare November 23, 2018 23:56
@1-alex98 1-alex98 force-pushed the feature/#280-ban-revoke-structure branch from a480525 to 2c49a1a Compare November 28, 2018 11:44
@Brutus5000
Copy link
Member

When I run this locally on Windows the following tests fail:
com.faforever.api.data.BanTest#canCreateBanInfoBasedOnModerationReportAsModerator
com.faforever.api.data.BanTest#canCreateBanInfoAsModerator
com.faforever.api.data.BanTest#canReadBanInfoAsModerator

Can you check?

@Brutus5000
Copy link
Member

Looks like it was a local gradle issue with not updating java-commons

@Brutus5000 Brutus5000 merged commit 15b5144 into develop Dec 14, 2018
@Brutus5000 Brutus5000 deleted the feature/#280-ban-revoke-structure branch September 5, 2019 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Brutus5000 Brutus5000 Brutus5000 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@1-alex98 @Brutus5000