Fix milestone semantic in Issue Search (#35)

If milestone was not defined in the rule, then only issues with milestones would be searched.

The semantic is now:

- No milestone set, consider issues without or without milestones
- Milestone is set consider only issues with the given milestone
- Milestone is * consider only issues with a milestone (any)
- Milestone is NONE consider only issues with no milestones set

Code was refactored and unit tests were added

Author: tspascoal

docs/Configuring.md

@@ -104,7 +104,10 @@ The configuration for issues has the following attributes:
 - `Assignee` (Optional. String). The user to search for issues assigned to. If omitted assignee(s) are not relevant.
 - `Author` (Optional. String). The user to search for issues created by. If omitted author is not relevant.
 - `Mention` (Optional. String). The user or team to search for issues mentioned in. If omitted mention is not relevant.
-- `Milestone` (Optional. Integer). The milestone **number** (not name) to search for issues in. If omitted milestone is not relevant (you can also specify `*` for all milestones).
+- `Milestone` (Optional. Integer). The milestone **number** (not name) to search for issues in. 
+  - If omitted milestone (or empty) is not relevant, issues with or without a milestone will be considered.
+  - Use `*` issues that have any milestone set.
+  - Use `NONE` to search for issues with no milestone.
 - `Labels` (Optional. Array of strings). The labels to search for issues with. If omitted labels are not relevant.
 - `OnlyCreatedBeforeWorkflowCreated` (optional. Boolean). If true only issues created before the workflow run was started will be considered. If false all issues will be considered.
 

src/Issues.Gate/Models/IssueGateIssues.cs

@@ -16,7 +16,6 @@ public class IssueGateIssues
         public string Milestone { get; set; }
         public List<string> Labels { get; set; }
         public string Message { get; set; }
-
         public bool OnlyCreatedBeforeWorkflowCreated { get; set; }
         public IList<string> Validate()
         {
@@ -44,9 +43,9 @@ public IList<string> Validate()
             {
                 errors.Add("If Mention is specified it cannot be empty");
             }
-            if (Milestone != null && string.IsNullOrWhiteSpace(Milestone))
+            if (Milestone != null && (Milestone != "NONE" && Milestone != "*" && !long.TryParse(Milestone, out long _)))
             {
-                errors.Add("If Milestone is specified it cannot be empty");
+                errors.Add("Milestone needs to be either a number, * or NONE");
             }
             if (Message != null && string.IsNullOrWhiteSpace(Message))
             {

src/Issues.Gate/Rules/IssueGateRulesEvaluator.cs

@@ -6,6 +6,10 @@
 using System.Text;
 using GitHubActions.Gates.Framework.Models;
 using Microsoft.Extensions.Logging;
+using System.Runtime.CompilerServices;
+
+// Needed for unit tests
+[assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")]
 
 namespace Issues.Gate.Rules
 {
@@ -22,7 +26,7 @@ public IssueGateRulesEvaluator(IGitHubAppClient client, ILogger log, IssuesConfi
             _log = log;
         }
 
-        public async Task<string> ValidateRules(string environment, Repo repository, long RunId)
+        public async virtual Task<string> ValidateRules(string environment, Repo repository, long RunId)
         {
             var rule = _configuration.GetRule(environment) ?? throw new RejectException($"No rule found for {environment} environment");
 
@@ -57,13 +61,13 @@ public async Task<string> ValidateRules(string environment, Repo repository, lon
             return comment.ToString();
         }
 
-        internal async Task ExecuteIssuesQuery(Repo Repository, IssueGateRule rule, DateTime? workflowCreatedAt, StringBuilder comment)
+        internal async virtual Task ExecuteIssuesQuery(Repo Repository, IssueGateRule rule, DateTime? workflowCreatedAt, StringBuilder comment)
         {
             var repo = rule.Issues.Repo != null ? new Repo(rule.Issues.Repo) : Repository;
 
             var graphQLQuery = $@"query($owner: String!, $repo: String!, $limit: Int, $states: [IssueState!] = OPEN, $assignee: String, $author: String, $mention: String, $milestone: String, $labels: [String!], $since: DateTime) {{
                         repository(owner: $owner, name: $repo) {{
-                            before: issues(first: $limit, states: $states, filterBy: {{ assignee: $assignee, createdBy: $author, mentioned: $mention, milestoneNumber: $milestone, labels: $labels}}) {{
+                            total: issues(first: $limit, states: $states, filterBy: {{ assignee: $assignee, createdBy: $author, mentioned: $mention, milestoneNumber: $milestone, labels: $labels}}) {{
                                       totalCount
                                     }}
                             after: issues(first: $limit, states: $states, filterBy: {{ since: $since, assignee: $assignee, createdBy: $author, mentioned: $mention, milestoneNumber: $milestone, labels: $labels}}) {{
@@ -72,27 +76,15 @@ internal async Task ExecuteIssuesQuery(Repo Repository, IssueGateRule rule, Date
                         }}   
                     }}";
 
-            var parameters = new
-            {
-                owner = repo.Owner,
-                repo = repo.Name,
-                limit = 0,
-                states = rule.Issues.State,
-                assignee = rule.Issues.Assignee,
-                author = rule.Issues.Author,
-                mention = rule.Issues.Mention,
-                milestone = rule.Issues.Milestone ?? "*",
-                labels = rule.Issues.Labels,
-                since = workflowCreatedAt
-            };
+            object parameters = BuildIssuesQueryParameters(repo, rule.Issues, workflowCreatedAt);
 
             try
             {
                 dynamic response = await _client.GraphQLAsync(graphQLQuery, parameters);
 
-                int nrIssues = response.data.repository.before.totalCount;
+                int nrIssues = response.data.repository.total.totalCount;
 
-                _log.LogInformation($"IssuesQuery: Before {nrIssues} After {response.data.repository.after.totalCount} issues with max {rule.Issues.MaxAllowed}");
+                _log.LogInformation($"IssuesQuery: Total {nrIssues} After {response.data.repository.after.totalCount} issues with max {rule.Issues.MaxAllowed}");
 
                 if (rule.Issues.OnlyCreatedBeforeWorkflowCreated)
                 {
@@ -118,8 +110,7 @@ internal async Task ExecuteIssuesQuery(Repo Repository, IssueGateRule rule, Date
                 RejectWithErrors("Issues", e);
             }
         }
-
-        internal async Task ExecuteSearchQuery(IssueGateRule rule, DateTime? workflowCreatedAt, StringBuilder comment)
+        internal async virtual Task ExecuteSearchQuery(IssueGateRule rule, DateTime? workflowCreatedAt, StringBuilder comment)
         {
             string query = BuildSearchQuery(rule, workflowCreatedAt);
 
@@ -161,6 +152,62 @@ internal static string BuildSearchQuery(IssueGateRule rule, DateTime? workflowCr
             return query;
         }
 
+        /// <summary>
+        /// Build the parameters for the Issues GraphQL query
+        /// 
+        /// Enforces the following semantic for milestones
+        /// <list type="bullet">
+        /// <item>If Milestone filter is specified then only issues with that given milestone are returned (use * for any milestone)</item>
+        /// <item>If Milestone filter is not specified then milestone value should be ignored(all other filter applies)</item>
+        /// <item>if Milestone filter is NONE then only issues with no milestones should be considered</item>
+        /// </list>
+        /// </summary>
+        /// <param name="repo"></param>
+        /// <param name="issues"></param>
+        /// <param name="workflowCreatedAt"></param>
+        /// <returns></returns>
+        internal static object BuildIssuesQueryParameters(Repo repo, IssueGateIssues issues, DateTime? workflowCreatedAt)
+        {
+            // milestone filter is ONLY added when is not null
+
+            // Very ugly code that doesn't scale if we need to apply same logic to other fields.
+            // Dealing with expandos was not nicer
+
+            var parameters = new
+            {
+                owner = repo.Owner,
+                repo = repo.Name,
+                limit = 0,
+                states = issues.State,
+                assignee = issues.Assignee,
+                author = issues.Author,
+                mention = issues.Mention,
+                milestone = issues.Milestone == "NONE" ? null : issues.Milestone,
+                labels = issues.Labels,
+                since = workflowCreatedAt
+            };
+
+            if (issues.Milestone == null)
+            {
+                // milestone is NOT added on purpose
+                return new
+                {
+                    parameters.owner,
+                    parameters.repo,
+                    parameters.limit,
+                    parameters.states,
+                    parameters.assignee,
+                    parameters.author,
+                    parameters.mention,
+                    parameters.labels,
+                    parameters.since
+                };
+            }
+
+            return parameters;
+        }
+
+
         private static void RejectWithErrors(string queryType, GraphQLException e)
         {
             StringBuilder mdErrors = new();