chibenwa commented on a change in pull request #346:
URL: https://github.com/apache/james-project/pull/346#discussion_r603822543
##########
File path:
server/data/data-jmap/src/main/java/org/apache/james/jmap/api/filtering/FilteringManagement.java
##########
@@ -29,14 +30,14 @@
public interface FilteringManagement {
- Publisher<Void> defineRulesForUser(Username username, List<Rule> rules);
+ Publisher<Optional<Version>> defineRulesForUser(Username username,
List<Rule> rules, Optional<Version> ifInState);
Review comment:
Why not simply `Publisher<Version>` as a return types ?
When don't we want to have a version?
Idem below...
##########
File path:
server/data/data-jmap/src/main/java/org/apache/james/jmap/api/filtering/Version.java
##########
@@ -41,6 +41,10 @@ public final boolean equals(Object o) {
return false;
}
+ public int getVersion() {
Review comment:
Can we call this `asInteger()` ?
##########
File path:
server/data/data-jmap/src/main/java/org/apache/james/jmap/api/filtering/impl/EventSourcingFilteringManagement.java
##########
@@ -56,8 +58,12 @@ public EventSourcingFilteringManagement(EventStore
eventStore) {
}
@Override
- public Publisher<Void> defineRulesForUser(Username username, List<Rule>
rules) {
- return Mono.from(eventSourcingSystem.dispatch(new
DefineRulesCommand(username, rules))).then();
+ public Publisher<Optional<Version>> defineRulesForUser(Username username,
List<Rule> rules, Optional<Version> ifInState) {
+ return Mono.from(eventSourcingSystem.dispatch(new
DefineRulesCommand(username, rules, ifInState)))
+ .then(Mono.from(eventStore.getEventsOfAggregate(new
FilteringAggregateId(username)))
+ .map(History::getVersionAsJava)
+ .map(eventIdOptional -> eventIdOptional.map(eventId -> new
Version(eventId.value()))
+ .or(Optional::empty)));
Review comment:
Here we want `Version.INITIAL` instead of `Optional::empty` ?
##########
File path:
server/data/data-jmap/src/main/java/org/apache/james/jmap/api/filtering/impl/FilteringAggregate.java
##########
@@ -81,6 +83,19 @@ private boolean shouldNotContainDuplicates(List<Rule> rules)
{
return uniqueIdCount == rules.size();
}
+ private boolean
shouldHaveTheSameVersionWithStoredVersion(Optional<Version> ifInState) {
+ return haveTheSameVersionWhenBothIsPresent(ifInState) ||
haveBothEmptyVersion(ifInState);
Review comment:
If `ifInState` is empty then we accept any version no?
Let me have a try:
```
private boolean expectedState(Optional<Version> ifInState) {
return ifInState.map(requestedVersion -> history.getVersionAsJava()
.orElse(Version.INITIAL)
.equals(requestVersion))
.orElse(true);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
