[1406] Filtering users who are not a member of the project should not…

… halt the filter
HubTurbo · Mar 27, 2016 · 244b43e · 244b43e
2 parents dc50061 + 99404ba
commit 244b43e
Show file tree

Hide file tree

Showing 10 changed files with 133 additions and 27 deletions.
diff --git a/src/main/java/backend/UpdateController.java b/src/main/java/backend/UpdateController.java
@@ -15,6 +15,7 @@
 import util.Futures;
 import util.HTLog;
 import util.events.FilterExceptionEvent;
+import util.events.FilterWarningEvent;
 
 import java.util.*;
 import java.util.concurrent.CompletableFuture;
@@ -150,6 +151,16 @@ private Map<FilterExpression, List<GuiElement>> processFilters(List<FilterExpres
 
                 List<GuiElement> processedElements = produceGuiElements(models, processedIssues);
 
+                List<String> warnings = allModelIssues.stream()
+                        .map(issue -> filterExprNoAlias.getWarnings(models, issue))
+                        .flatMap(List::stream)
+                        .distinct()
+                        .collect(Collectors.toList());
+
+                if (!warnings.isEmpty()) {
+                    Platform.runLater(() -> UI.events.triggerEvent(new FilterWarningEvent(filterExpr, warnings)));
+                }
+
                 processed.put(filterExpr, processedElements);
             } catch (FilterException e) {
                 Platform.runLater(() -> UI.events.triggerEvent(new FilterExceptionEvent(filterExpr, e.getMessage())));

diff --git a/src/main/java/filter/expression/Conjunction.java b/src/main/java/filter/expression/Conjunction.java
@@ -76,6 +76,15 @@ public void applyTo(TurboIssue issue, IModel model) throws QualifierApplicationE
         right.applyTo(issue, model);
     }
 
+    @Override
+    public List<String> getWarnings(IModel model, TurboIssue issue) {
+        List<String> leftWarnings = left.getWarnings(model, issue);
+        List<String> rightWarnings = right.getWarnings(model, issue);
+        List<String> result = leftWarnings;
+        result.addAll(rightWarnings);
+        return result;
+    }
+
     @Override
     public List<QualifierType> getQualifierTypes() {
         ArrayList<QualifierType> list = new ArrayList<>();

diff --git a/src/main/java/filter/expression/Disjunction.java b/src/main/java/filter/expression/Disjunction.java
@@ -62,6 +62,15 @@ public void applyTo(TurboIssue issue, IModel model) throws QualifierApplicationE
         assert false;
     }
 
+    @Override
+    public List<String> getWarnings(IModel model, TurboIssue issue) {
+        List<String> leftWarnings = left.getWarnings(model, issue);
+        List<String> rightWarnings = right.getWarnings(model, issue);
+        List<String> result = leftWarnings;
+        result.addAll(rightWarnings);
+        return result;
+    }
+
     @Override
     public List<QualifierType> getQualifierTypes() {
         ArrayList<QualifierType> list = new ArrayList<>();

diff --git a/src/main/java/filter/expression/FilterExpression.java b/src/main/java/filter/expression/FilterExpression.java
@@ -28,6 +28,11 @@ public interface FilterExpression {
 
     void applyTo(TurboIssue issue, IModel model) throws QualifierApplicationException;
 
+    // Walks the syntax tree to get all problems with the input which are not severe
+    // enough to cause an error.
+
+    List<String> getWarnings(IModel model, TurboIssue issue);
+
     // Walks the syntax tree to get all the qualifier types that appear.
 
     List<QualifierType> getQualifierTypes();

diff --git a/src/main/java/filter/expression/Negation.java b/src/main/java/filter/expression/Negation.java
@@ -55,6 +55,11 @@ public void applyTo(TurboIssue issue, IModel model) throws QualifierApplicationE
         assert false;
     }
 
+    @Override
+    public List<String> getWarnings(IModel model, TurboIssue issue) {
+        return expr.getWarnings(model, issue);
+    }
+
     @Override
     public List<QualifierType> getQualifierTypes() {
         return expr.getQualifierTypes();

diff --git a/src/main/java/filter/expression/Qualifier.java b/src/main/java/filter/expression/Qualifier.java
@@ -353,7 +353,7 @@ public boolean isSatisfiedBy(IModel model, TurboIssue issue, MetaQualifierInfo i
         case LABEL:
             return labelsSatisfy(model, issue);
         case AUTHOR:
-            return authorSatisfies(model, issue);
+            return authorSatisfies(issue);
         case ASSIGNEE:
             return assigneeSatisfies(model, issue);
         case INVOLVES:
@@ -429,6 +429,30 @@ public void applyTo(TurboIssue issue, IModel model) throws QualifierApplicationE
         }
     }
 
+    @Override
+    public List<String> getWarnings(IModel model, TurboIssue issue) {
+        switch (type) {
+            case AUTHOR:
+            case ASSIGNEE:
+            case INVOLVES:
+                return getWarningsForTypeAuthorOrAssignee(model, issue);
+            default:
+                return new ArrayList<>();
+        }
+    }
+
+    /**
+     * If the user referred in the qualifier is not a contributor of the repository of the TurboIssue,
+     * it returns a List<String> containing a warning that explains so, or an empty List<String> otherwise.
+     */
+    private List<String> getWarningsForTypeAuthorOrAssignee(IModel model, TurboIssue issue) {
+        List<String> result = new ArrayList<>();
+        if (content.isPresent() && !model.isUserInRepo(issue.getRepoId(), content.get())) {
+            result.add(String.format(USER_WARNING_ERROR_FORMAT, content.get(), issue.getRepoId()));
+        }
+        return result;
+    }
+
     @Override
     public boolean canBeAppliedToIssue() {
         return true;
@@ -843,33 +867,22 @@ private boolean assigneeSatisfies(IModel model, TurboIssue issue) {
 
         if (!assignee.isPresent()) return false;
 
-        enforceUserInRepoCondition(model, issue.getRepoId(), content.get());
-
         String content = this.content.get().toLowerCase();
         String login = assignee.get().getLoginName() == null ? "" : assignee.get().getLoginName().toLowerCase();
         String name = assignee.get().getRealName() == null ? "" : assignee.get().getRealName().toLowerCase();
 
         return login.contains(content) || name.contains(content);
     }
 
-    private boolean authorSatisfies(IModel model, TurboIssue issue) {
+    private boolean authorSatisfies(TurboIssue issue) {
         if (!content.isPresent()) return false;
 
-        enforceUserInRepoCondition(model, issue.getRepoId(), content.get());
-
         String creator = issue.getCreator();
         return creator.toLowerCase().contains(content.get().toLowerCase());
     }
 
-    private void enforceUserInRepoCondition(IModel model, String repoId, String userName) {
-        boolean shouldWarnUser = !model.isUserInRepo(repoId, userName);
-        if (shouldWarnUser) {
-            throw new SemanticException(String.format(USER_WARNING_ERROR_FORMAT, userName, repoId));
-        }
-    }
-
     private boolean involvesSatisfies(IModel model, TurboIssue issue) {
-        return authorSatisfies(model, issue) || assigneeSatisfies(model, issue);
+        return authorSatisfies(issue) || assigneeSatisfies(model, issue);
     }
 
     public static boolean labelMatches(String input, String candidate) {

diff --git a/src/main/java/ui/issuepanel/FilterPanel.java b/src/main/java/ui/issuepanel/FilterPanel.java
@@ -106,6 +106,7 @@ private void setUpEventHandler() {
         ui.registerEvent((ApplyingFilterEventHandler) this::startLoadingAnimationIfApplicable);
         ui.registerEvent((AppliedFilterEventHandler) this::stopLoadingAnimationIfApplicable);
         ui.registerEvent((FilterExceptionEventHandler) this::handleFilterException);
+        ui.registerEvent((FilterWarningEventHandler) this::handleFilterWarning);
     }
 
     private final ModelUpdatedEventHandler onModelUpdate = e -> {
@@ -227,11 +228,20 @@ private void applyFilterExpression(FilterExpression filter) {
 
     protected abstract void stopLoadingAnimationIfApplicable(AppliedFilterEvent e);
 
+    private void handleFilterWarning(FilterWarningEvent e) {
+        if (!e.filterExpr.equals(getCurrentFilterExpression())) return;
+        showWarning(e.warnings.get(0));
+    }
+
     private void handleFilterException(FilterExceptionEvent e) {
         if (!e.filterExpr.equals(getCurrentFilterExpression())) return;
         emptyFilterAndShowError(e.exceptionMessage);
     }
 
+    private void showWarning(String warning) {
+        UI.status.displayMessage(getUniquePanelName(panelMenuBar.getPanelName()) + ": " + warning);
+    }
+
     private void emptyFilterAndShowError(String exceptionMessage) {
         this.applyFilterExpression(Qualifier.EMPTY);
         filterTextField.setStyleForInvalidFilter();

diff --git a/src/main/java/util/events/FilterWarningEvent.java b/src/main/java/util/events/FilterWarningEvent.java
@@ -0,0 +1,19 @@
+package util.events;
+
+import filter.expression.FilterExpression;
+
+import java.util.List;
+
+/**
+ * This class indicates that there are warnings for a particular FilterExpression
+ */
+public class FilterWarningEvent extends Event {
+    public final FilterExpression filterExpr;
+    public final List<String> warnings;
+
+    public FilterWarningEvent(FilterExpression filterExpr, List<String> warnings) {
+        this.filterExpr = filterExpr;
+        this.warnings = warnings;
+        assert !warnings.isEmpty();
+    }
+}
diff --git a/src/main/java/util/events/FilterWarningEventHandler.java b/src/main/java/util/events/FilterWarningEventHandler.java
@@ -0,0 +1,9 @@
+package util.events;
+
+import com.google.common.eventbus.Subscribe;
+
+@FunctionalInterface
+public interface FilterWarningEventHandler extends EventHandler {
+    @Subscribe
+    void handle(FilterWarningEvent e);
+}
diff --git a/src/test/java/tests/FilterEvalTests.java b/src/test/java/tests/FilterEvalTests.java
@@ -7,6 +7,7 @@
 import java.time.LocalDateTime;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.List;
 import java.util.Optional;
 
 import org.junit.Rule;
@@ -828,14 +829,22 @@ public void filterLabelMatching() {
     @Test
     public void processQualifier_useInvalidUsername_getUsernameWarning() {
         TurboUser user = new TurboUser(REPO, "fox", "charlie");
-        IModel model = TestUtils.singletonModel(new Model(REPO,
-                                                            new ArrayList<>(),
-                                                            new ArrayList<>(),
-                                                            new ArrayList<>(),
-                                                            new ArrayList<>(Arrays.asList(user))));
-        verifySemanticException(model, "involves:bob", String.format(USER_WARNING_ERROR_FORMAT, "bob", REPO));
-        verifySemanticException(model, "involves:foxx", String.format(USER_WARNING_ERROR_FORMAT, "bob", REPO));
-        verifySemanticException(model, "author:alice", String.format(USER_WARNING_ERROR_FORMAT, "alice", REPO));
+        IModel model = TestUtils.singletonModel(createModelFromUsers(REPO, user));
+        verifyUserWarning(model, "involves:bOb",
+                            Arrays.asList(String.format(USER_WARNING_ERROR_FORMAT, "bOb", REPO)));
+        verifyUserWarning(model, "involves:foxX",
+                            Arrays.asList(String.format(USER_WARNING_ERROR_FORMAT, "foxX", REPO)));
+        verifyUserWarning(model, "author:alice",
+                            Arrays.asList(String.format(USER_WARNING_ERROR_FORMAT, "alice", REPO)));
+    }
+
+    @Test
+    public void processQualifier_useValidUsername_noUsernameWarning() {
+        TurboUser user = new TurboUser(REPO, "fox", "charlie");
+        IModel model = TestUtils.singletonModel(createModelFromUsers(REPO, user));
+        verifyUserWarning(model, "involves:fOX", new ArrayList<>());
+        verifyUserWarning(model, "assignee:FOX", new ArrayList<>());
+        verifyUserWarning(model, "assignee:CHAR", new ArrayList<>());
     }
 
     @Test
@@ -846,11 +855,7 @@ public void processQualifier() {
         TurboIssue issue1 = new TurboIssue(REPO, 1, "title", "alice", LocalDateTime.now(), false);
         TurboIssue issue2 = new TurboIssue(REPO, 1, "title", "bob", LocalDateTime.now(), false);
         issue1.setAssignee(user3);
-        IModel model = TestUtils.singletonModel(new Model(REPO,
-                new ArrayList<>(),
-                new ArrayList<>(),
-                new ArrayList<>(),
-                new ArrayList<>(Arrays.asList(user1, user2, user3))));
+        IModel model = TestUtils.singletonModel(createModelFromUsers(REPO, user1, user2, user3));
         assertTrue(Qualifier.process(model, Parser.parse("assignee:ox"), issue1));
         assertTrue(Qualifier.process(model, Parser.parse("author:alice"), issue1));
         assertFalse(Qualifier.process(model, Parser.parse("assignee:charlie"), issue2));
@@ -881,4 +886,15 @@ private void verifyQualifierContentError(QualifierType type, String invalidInput
         verifySemanticException(empty, invalidInput, String.format(SemanticException.ERROR_MESSAGE,
                                                                     type, type.getDescriptionOfValidInputs()));
     }
+
+    private void verifyUserWarning(IModel model, String input, List<String> expectedWarnings) {
+        TurboIssue issue = new TurboIssue(REPO, 1, "title");
+        FilterExpression filterExpr = Parser.parse(input);
+        List<String> warnings = filterExpr.getWarnings(model, issue);
+        assertEquals(expectedWarnings, warnings);
+    }
+
+    private Model createModelFromUsers(String name, TurboUser... users) {
+        return new Model(name, new ArrayList<>(), new ArrayList<>(), new ArrayList<>(), Arrays.asList(users));
+    }
 }