-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add Ladder Map Voting stuff #60
Conversation
Attention Voting is partly broken !!! on current devlop of java-api. I made the necessary change and will reference the other PR soon. Due to changes made lately |
75badc8
to
45edd6d
Compare
depends on |
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.
Shame on me. I wasn't aware I broke develop. Will add a build pipeline to prevent this in the future.
For now I just reviewed the code itself and did not actually test the functionality.
result.remove(map); | ||
} | ||
} | ||
log.trace("found {} maps", preResult.size()); |
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.
What are you trying to achieve here? If this is about erasing duplicates then put it into a Set<Map>
outside of the method or in a new method. We are using the id of all elide entities to check for equality.
Furthermore: You are changing the semantics of this method, which will break other parts of the ui. Assume someone assigns different versions of the same map to a pool. You will never be able to fix this if you prematurely remove the duplicates. It may have been a mistake to return the Map
instead of the MapVersion
here, because this is ambigious. We could fix that instead.
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 Set
As you set I use the id instead of equals because equal method won't work here, I check that. We
might wanna change that as well. - Returning Map instead of MapVersion
I changed it because of problems that were there. You add one map version, the map was added with all its versions and version that were not included were shown empty. And if you added two versions of the same map the map was shown twice. So I corrected all those fallacies. I only show versions that are in the ladder map pool and I don't show map duplicates any longer. From the way the tree view is structured it needs Map objects and also I did not change the return type I only corrected the data output of the method.
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 don't think I should change anything here should I?
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.
This test shows me that equality is checked by id only:
@Test
public void checkEqualityOfMapById() {
MapVersion m1 = (MapVersion)new MapVersion()
.setRanked(true)
.setId("1");
MapVersion m1_copy = (MapVersion)new MapVersion()
.setRanked(false)
.setId("1");
assertTrue(Objects.equals(m1, m1_copy));
assertFalse(m1 == m1_copy);
}
A set has to work. The above code needs to be simplified.
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.
What you are looking at it the MapVersion equal code which is not relevant because it is a Set, the correct code is:
public boolean equals(Object o) {
if (o == this) {
return true;
} else if (!(o instanceof Map)) {
return false;
} else {
Map other = (Map)o;
return other.canEqual(this);
}
}
protected boolean canEqual(Object other) {
return other instanceof Map;
}
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.
Shall I change faf-commons????
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.
Oh that was due to some Lombok annotation leftovers. Fixed in FAForever/faf-java-commons@6623f1f
List<Message> messages = fafApi.getAll(ElideNavigator.of(Message.class).collection() | ||
.addFilter(ElideNavigator.qBuilder().string("region").eq(message.getRegion())) | ||
.addFilter(ElideNavigator.qBuilder().string("language").eq(message.getLanguage())) | ||
.addFilter(ElideNavigator.qBuilder().string("key").eq(message.getKey()))); |
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.
My bad. This should not be addFilter
, but setFilter
instead in the ElideNavigator. You can't add multiple filters, each one will override the previous.
Use the qBuilders .and()
concatenation instead.
@@ -117,7 +120,7 @@ public VotingQuestion create(VotingQuestion votingQuestion) { | |||
//region choices | |||
public List<VotingChoice> getAllChoicesFromApi() { | |||
log.debug("Retrieving all choices"); | |||
List<VotingChoice> result = fafApi.getAll(ElideNavigator.of(VotingChoice.class).collection()); | |||
List<VotingChoice> result = fafApi.getAll(ElideNavigator.of(VotingChoice.class).collection().addIncludeOnCollection("votingQuestion")); |
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.
Please follow the style as in line 79: if there are more operations put them in new lines.
@@ -1093,7 +1092,7 @@ public static void buildMessagesTable(TableView<MessageFx> messageTableView, Mes | |||
messageTableView.setEditable(true); | |||
HashMap<TableColumn<MessageFx, ?>, Function<MessageFx, ?>> extractors = new HashMap<>(); | |||
|
|||
TableColumn<MessageFx, Number> idColumn = new TableColumn<>("ID"); | |||
TableColumn<MessageFx, Integer> idColumn = new TableColumn<>("ID"); |
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.
Please stick with number. There are some limitations in JavaFX where you get weird errors if you use Integer. I don't recall what is was but it is hard to track.
|
||
try { | ||
if (votingService.create(votingSubjectFX) == null) { | ||
if (votingService.create(votingSubject) == null) { |
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.
We are separating the api entities from the fx beans on purpose. There is a strict separation between API dtos and our UI entities. Revert these changes.
votingService.updateSubject(selectedItem); | ||
votingSubject.setId(selectedItem.getId()); | ||
votingSubject.setRevealWinner(true); | ||
votingService.update(votingSubject); |
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.
Same here. Stick with the FX beans in the controllers.
} | ||
if (!messages.isEmpty()) { | ||
log.info("deleting existing message ''{}''", message); | ||
messages.forEach(message1 -> deleteMessage(messagesMapper.map(message1))); |
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.
Please us a proper variable name instead of message1
295d742
to
fa8dc78
Compare
Set<Map> result = ladder1v1MapVersions.stream() | ||
.map(mapVersion -> { | ||
Map map = mapVersion.getMap(); | ||
map.setVersions(map.getVersions().stream().filter(ladder1v1MapVersions::contains).collect(Collectors.toList())); |
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.
Proper formatting now and we're good.
fa8dc78
to
510d9d8
Compare
like this |
Should this work against develop-api? Creating a vote fails with an error without using the regular exception message bo and then it throws a success message afterwards. Both needs to be fixed. |
As I wrote early no it does not work against develop api |
depends on |
510d9d8
to
a63e8d8
Compare
fixed that false positive dialog. was missing a return in error case;) |
a63e8d8
to
1ad67be
Compare
@@ -84,6 +83,8 @@ public FafApiCommunicationService(ResourceConverter resourceConverter, Applicati | |||
.additionalMessageConverters(jsonApiMessageConverter) | |||
.errorHandler(jsonApiErrorHandler) | |||
.rootUri(apiBaseUrl); | |||
headers = new HttpHeaders(); |
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.
@bukajsytlos @Brutus5000 for post i got headers working, however i still need to due it for patch. Do you know if there is a way to set headers in general?
1ad67be
to
6793222
Compare
try { | ||
authorizedLatch.await(); | ||
return restOperations.exchange(url, HttpMethod.PATCH, new HttpEntity<>(object), routeBuilder.getDtoClass()).getBody(); | ||
return restOperations.exchange(url, HttpMethod.PATCH, new HttpEntity<>(object, httpHeaders), routeBuilder.getDtoClass()).getBody(); | ||
} catch (Throwable t) { | ||
applicationEventPublisher.publishEvent(new FafApiFailModifyEvent(t, routeBuilder.getDtoClass(), url)); | ||
throw t; |
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.
How do i do it for the patch method blow @bukajsytlos @Brutus5000
src/main/java/com/faforever/moderatorclient/api/FafApiCommunicationService.java
Show resolved
Hide resolved
The changes you made do not work @Brutus5000
|
I would suspect it is because you only set the content type but not what it consumes 🤔 |
|
|
When do you run into the error? Is the stacktrace API or client? |
API stacktrace |
I think your changes are good @bukajsytlos I will test it |
Works fine however ladder vote broke but i am sure i can quickly fix that |
a5d9a52
to
f8bd337
Compare
All good to go and finally working again |
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.
Finally. Let's see which surprises production will bring us :D
Fixes #52