RFR: 1951: Refactor Issue class hierarchy

[Zhao Song]

On Fri, 16 Jun 2023 22:05:31 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

> The class hierarchy around the (org.openjdk.skara.issuetracker.)Issue interface is weird and cumbersome to work with. This interface is representing an Issue as returned from an IssueTracker. It's implemented by `JiraIssue` (and `TestIssue` for testing purposes). It's also extended by `PullRequest`, which makes it awkward to add Jira/bug tracker specific functionality to it since we must then also implement that for all `PullRequest` implementations (usually by just throwing a runtime exception). It's rare to use `Issue` as something that can be either a `PullRequest` or an actual issue, so sharing this interface isn't adding much value.
> 
> In addition to this, there is also an unrelated class called Issue (org.openjdk.skara.vcs.openjdk.Issue) in the vcs module. This class just represents the OpenJDK interpretation of an issue in the context of a commit message. These classes are however used together sometimes, which is causing a lot of confusion.
> 
> I would like to improve this situation. My proposal is to introduce a new interface `IssueTrackerIssue` which is a sub type of `Issue` that contains methods specific to bug trackers (or at least Jira). In practice right now this is links and properties. The name reflects that this is the kind of issue returned from an `IssueTracker` as opposed to an Issue from just any kind of `Forge`. Having that in an interface enables us to also define a specific test implementation `TestIssueTrackerIssue` that mimics our `JiraIssue` implementation without clashing with `PullRequest` and its implementations. With this new type name, we can also get around the clash when having to deal with the vcs Issue type. This solution retains the shared super interface `Issue` between `IssueTrackerIssue` and `PullRequest`. I'm also refactoring `TestIssue` and its subclasses to reflect this new hierarchy.
> 
> The patch is pretty big, which is why I'm doing this in isolation. No functionality should change.

I think this patch is good, and your design of the class hierarchy makes a lot of sense. I just have these minor suggestions. Additionally, copyrights in some updated files need to be updated.

Furthermore, regarding setting or getting the state of an IssueTrackerIssue or a Pull Request, we currently utilize `Issue.State.XXX`. However, I think it might be better to use `IssueTrackerIssue.State.XXX` and `PullRequest.State.XXX`.

bots/jep/src/test/java/org/openjdk/skara/bots/jep/JEPBotTests.java line 27:

> 25: import org.junit.jupiter.api.Test;
> 26: import org.junit.jupiter.api.TestInfo;
> 27: import org.openjdk.skara.issuetracker.Issue;

We could remove this ` import org.openjdk.skara.issuetracker.Issue;`

bots/pr/src/main/java/org/openjdk/skara/bots/pr/JEPCommand.java line 33:

> 31: import java.util.List;
> 32: import java.util.Optional;
> 33: import org.openjdk.skara.issuetracker.IssueTrackerIssue;

`import org.openjdk.skara.issuetracker.Issue;` in line 28 can be removed

bots/pr/src/test/java/org/openjdk/skara/bots/pr/IssueTests.java line 435:

> 433:             var editHash = CheckableRepository.appendAndCommit(localRepo);
> 434:             localRepo.push(editHash, author.authenticatedUrl(), "edit", true);
> 435:             var issue1 = (TestIssueTrackerIssue)issues.createIssue("First", List.of("Hello"), Map.of());

Suggestion:

            var issue1 = (TestIssueTrackerIssue) issues.createIssue("First", List.of("Hello"), Map.of());

bots/pr/src/test/java/org/openjdk/skara/bots/pr/IssueTests.java line 476:

> 474:             var editHash = CheckableRepository.appendAndCommit(localRepo);
> 475:             localRepo.push(editHash, author.authenticatedUrl(), "edit", true);
> 476:             var issue1 = (TestIssueTrackerIssue)issues.createIssue("First", List.of("Hello"), Map.of());

Suggestion:

            var issue1 = (TestIssueTrackerIssue) issues.createIssue("First", List.of("Hello"), Map.of());

jbs/src/main/java/org/openjdk/skara/jbs/Backports.java line 257:

> 255:         log.fine("Searching for fixed issue with fix version matching /" + versionPattern + "/ "
> 256:                 + " for primary issue " + primary.id());
> 257:         return Stream.concat(Stream.of(primary).filter(Issue::isFixed), findBackports(primary, true).stream())

I think `Issue` here could be changed to `IssueTrackerIssue`. Therefore, we don't need to import `org.openjdk.skara.issuetracker.Issue` in this class anymore.

test/src/main/java/org/openjdk/skara/test/TestIssueStore.java line 59:

> 57:         this.title = title;
> 58:         this.body = String.join("\n", body);
> 59: 

Maybe this line is extra

test/src/main/java/org/openjdk/skara/test/TestPullRequest.java line 69:

> 67:     protected TestPullRequest copy() {
> 68:         return new TestPullRequest(store(), targetRepository);
> 69:     }

Do we still need this method? There is no usage of this method

-------------

PR Review: https://git.openjdk.org/skara/pull/1534#pullrequestreview-1488676752
PR Review Comment: https://git.openjdk.org/skara/pull/1534#discussion_r1235657242
PR Review Comment: https://git.openjdk.org/skara/pull/1534#discussion_r1235717885
PR Review Comment: https://git.openjdk.org/skara/pull/1534#discussion_r1235720607
PR Review Comment: https://git.openjdk.org/skara/pull/1534#discussion_r1235721454
PR Review Comment: https://git.openjdk.org/skara/pull/1534#discussion_r1235602742
PR Review Comment: https://git.openjdk.org/skara/pull/1534#discussion_r1235645147
PR Review Comment: https://git.openjdk.org/skara/pull/1534#discussion_r1235650313