Add Git Panel #7235

mmorhun · 2017-11-07T15:02:47Z

What does this PR do?

Adds git panel to Che. The panel contains list of git repositories in workspace with label for each repository which shows number of changed files in it.
If selected repository have some changes, they will be displayed on the panel. Double click on changed file will show its diff.
The panel is updated dynamically and always keep actual state.

To quick show git panel Alt + g shortcut could be used.

Selenium test is added for most common use cases.

What issues does this PR fix or reference?

#5128
#7022

Git panel added into Che.

Release Notes

Git panel with list of repositories and changed files list is added into Che.

Docs PR

CHE-6361: Add git panel prototype

…ges (#7073)

tolusha · 2017-11-07T15:25:50Z

...in-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/panel/GitPanelPresenter.java

+    this.changes = new HashMap<>();
+
+    for (Project project : appContext.getProjects()) {
+      if (projectUnderGit(project)) {


isGitProject

To me isProjectUnderGit looks better. Do you mind?

tolusha · 2017-11-07T15:27:01Z

...in-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/panel/GitPanelPresenter.java

+  }
+
+  /** Queries from server all data needed to initialize the panel. */
+  private void loadPanelData() {


resolveGitRepositories

I think, that current name is better, because there is noting to resolve here, just request data from wsagent.

exposeGitRepositories

azatsarynnyy · 2017-11-07T19:04:07Z

...he-plugin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/GitEventsHandler.java

+
+  private static final String EVENT_GIT_FILE_CHANGED = "event/git-change";
+  private static final String EVENT_GIT_STATUS_CHANGED = "event/git/status-changed";
+  private static final String EVENT_GIT_CHECKOUT = "event/git-checkout";


it would be nice to have these constants shared between server and client code

Yes, it would be great. But the issue is common for all rpc channels and probably we have to find general solution.

azatsarynnyy · 2017-11-07T19:14:54Z

...-plugin-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/panel/GitPanelView.java

+  interface ActionDelegate extends BaseActionDelegate {
+
+    /**
+     * Invoked each time when user changes selection in repositories list. Returns null if no


Returns null if no selected items

it returns void

Thank you! I meant argument...

tolusha · 2017-11-08T10:35:56Z

...in-git-ext-git/src/main/java/org/eclipse/che/ide/ext/git/client/panel/GitPanelPresenter.java

+  }
+
+  @Override
+  public void onFileChanged(String endpointId, FileChangedEventDto dto) {


dto -> fileChangedEvent

vinokurig

It would be great to use icons from http://fontawesome.io instead of your SVG icons where possible

mmorhun · 2017-11-09T16:04:56Z

ci-test

codenvy-ci · 2017-11-10T00:30:27Z

ci-test build report:
Build details
Test report
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun · 2017-11-10T08:23:38Z

@slemeur we are going to merge it, do you have any concerns?

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

mmorhun · 2017-11-10T08:40:27Z

ci-test

slemeur · 2017-11-10T08:51:50Z

Yes.

The PR must not be merged: In this state, the panel can't be delivered to end-users!

As I told in few discussions, there are few problems to fix in what the panel is showing currently:

It misses information about the branch name: so when I see the git status, user does not know on which branch he is working on.
Butons: If you check the requirements, the buttons to manage the list's settings are not handled this way, and they have a different style.
Icons: git icon is incorrect. in the mockup there is no icon for the repositories.

Then, there is a bigger issue:
The goal of the panel is to fasten the interactions with the mostly used git features such as fetch, pull, commit, push. At this moment, the panel does not reach this goal and does not complete the user stories.
I would say, if it had only the ability to commit, we could consider merging it - but as it is currently, it is really too limited and there is very narrow value to merge it.

The current state, is really a WIP. I would say that you might need to consider feature flag to introduce this kind of WIP/experimental feature.

dmytro-ndp · 2017-11-10T10:55:17Z

@mmorhun: if you decide to merge it with turned-off Git-panel, along with selenium test GitPanelTest.java for the panel, please, make sure that this test is excluded from test execution properly, that means:

GitPanelTest.java excluded from CheSuite.xml;
all tests inside the GitPanelTest.java disabled (that are annotated with @Test(enabled = false)) to prevent run in package mode.

Anyway, I don't like an idea of presence of selenium tests, which are aimed to test absent functionality, so as they should being supported as others tests, but this support would be useless.

codenvy-ci · 2017-11-10T12:59:12Z

ci-test build report:
Build details
Test report
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

mmorhun · 2017-11-10T13:51:44Z

We decided to merge this changes, but don't include it in IDE functionality. This is due to refactoring of some git events mechanism which probably will have merge conflicts very soon. To avoid hard merging and taking into account that we don't refuse git panel we are going to merge git panel code too.

mmorhun · 2017-11-10T13:54:27Z

@dmytro-ndp can you elaborate please, what king of support we have to do for disabled tests?

dmytro-ndp · 2017-11-10T14:07:04Z

We should support various things which GitPanelTest uses:

methods of page objects and helper classes (Loader, TestProjectServiceClient, Wizard, Menu, ProjectExplorer, CodenvyEditor, MachineTerminal, AskDialog, AskForValueDialog, TestUserPreferencesServiceClient, Git, GitCompare, GitPanel)
resources ("/projects/node-js-simple", "/projects/ProjectWithDifferentTypeOfFiles", "/projects/plain-java-project").

That means that we can't change them without the risk to break the test logic, so as we can't ensure that without turning on the Git Panel and run the test.

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

dmytro-ndp · 2017-11-10T14:23:14Z

selenium/che-selenium-test/src/test/java/org/eclipse/che/selenium/git/GitPanelTest.java

+  @Inject private AskDialog askDialog;
+  @Inject private AskForValueDialog askForValueDialog;
+  @Inject private TestUserPreferencesServiceClient testUserPreferencesServiceClient;
+  @Inject private org.eclipse.che.selenium.pageobject.git.Git git;


Add org.eclipse.che.selenium.pageobject.git.Git into the import section.

dmytro-ndp · 2017-11-10T14:24:41Z

selenium/che-selenium-test/src/test/java/org/eclipse/che/selenium/git/GitPanelTest.java

+    commitAllChangesInProject(JAVA_SPRING_GIT_PROJECT_NAME, "initial commit");
+  }
+
+  @Test(priority = 1)


It's better to have independent tests inside the test class.

dmytro-ndp · 2017-11-10T14:28:01Z

selenium/che-selenium-test/src/test/java/org/eclipse/che/selenium/git/GitPanelTest.java

+  }
+
+  /**
+   * Converts file name to editor tab title for java projects.


Example of conversion will be quite useful here.

dmytro-ndp · 2017-11-10T14:29:40Z

selenium/che-selenium-test/src/main/java/org/eclipse/che/selenium/pageobject/git/GitPanel.java

+  public void waitRepositoryPresent(String repository) {
+    loadWait.until(
+        ExpectedConditions.presenceOfElementLocated(
+            By.xpath(String.format(Locators.REPOSITORY_BY_NAME, repository))));


It's better to use static imports to improve readability.
Compare:
By.xpath(String.format(Locators.REPOSITORY_BY_NAME, repository))));
and
xpath(format(REPOSITORY_BY_NAME, repository))));

Well, I am just afraid that many static imports could make our code less clear.

Believe me - in case of Page Object many static imports make code much clear. @tolusha can confirm it.

I won't confirm that. It is matter of personal preferences and certain cases

@tolusha: hm, I thought you was supporter of usage of static imports, basing on your earlier comments like this https://github.com/codenvy-legacy/qa/pull/644#discussion_r104435935 :-|
Anyway, usage of static import for constants like Locators.REPOSITORY_BY_NAME is obvious improvement so as Locators is just an interface for constants.

mmorhun · 2017-11-10T14:36:18Z

We should support various things which GitPanelTest uses
methods of page objects and helper classes

We have to support them anyway, regardless of GitPanelTest.

resources ("/projects/node-js-simple", "/projects/ProjectWithDifferentTypeOfFiles", "/projects/plain-java-project")

I didn't add new resources, just reused existing. By the way, what kind of support you meant?

As about the git panel test. The functionality will be hidden... nothing to test. We'll retest the scenario when include git panel functionality. I think this is optimal way.

mmorhun · 2017-11-10T14:47:12Z

ci-test

dmytro-ndp · 2017-11-10T22:29:07Z

@mmorhun:

By the way, what kind of support you meant?

By support I mean keeping track of changes in methods of Page Objects and libraries, and content of resources which are used by TestGitPanel. When we will be changing the logic or signature of methods of dependent classes, or content of resources, we will need to ensure that changes don't break the test. To do this, we ought to turn-on the git panel, to run the test and potentially to correct it, and it will take a time. Otherwise this test will "dead" and we will remove it later anyway.

mmorhun added 12 commits October 12, 2017 10:03

CHE-6361: Add prototype of git panel (#6660)

78d9957

CHE-6361: Add git panel prototype

Merge branch 'master' into git-panel

2bded95

Merge branch 'master' into git-panel

adc8a5e

CHE-6608: Synchronize list of repositories and the list of local chan…

b724ce7

…ges (#7073)

Merge branch 'che6' into git-panel

b3bd214

Fixes after merge

bb32ffb

Merge branch 'che6' into git-panel

fbdc8b0

Small fixes after merge

645a27c

Merge branch 'che6' into git-panel

ba1987a

Merge branch 'che6' into git-panel

01b230a

Add selenium test. Fix small bugs

349f921

Delete unused field

08a8b3d

mmorhun requested review from azatsarynnyy, vparfonov, slemeur, musienko-maxim, tolusha, vzhukovs, AndrienkoAleksandr and vinokurig November 7, 2017 15:02

benoitf added target/che6 kind/task Internal things, technical debt, and to-do tasks to be performed. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Nov 7, 2017

tolusha reviewed Nov 7, 2017

View reviewed changes

mmorhun requested review from tolusha and vitaliy-guliy November 7, 2017 15:47

azatsarynnyy reviewed Nov 7, 2017

View reviewed changes

Fix test suite syntax

c0806f6

tolusha reviewed Nov 8, 2017

View reviewed changes

vinokurig approved these changes Nov 9, 2017

View reviewed changes

vzhukovs approved these changes Nov 9, 2017

View reviewed changes

vitaliy-guliy approved these changes Nov 9, 2017

View reviewed changes

mmorhun added 2 commits November 10, 2017 09:35

Move colors to theme. Remove _ from gwt ids

f16b4d6

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

Merge branch 'che6' into git-panel

5f97964

Fix selenium test

48fdfb4

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

musienko-maxim approved these changes Nov 10, 2017

View reviewed changes

Hide git panel. Disable git panel test

5374b60

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

dmytro-ndp reviewed Nov 10, 2017

View reviewed changes

Add import for Git test

3876691

dmytro-ndp reviewed Nov 10, 2017

View reviewed changes

mmorhun merged commit 46ddf5e into che6 Nov 13, 2017

mmorhun deleted the git-panel branch November 13, 2017 07:24

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 13, 2017

benoitf added this to the 6.0.0-M2 milestone Nov 13, 2017

Add Git Panel #7235

Add Git Panel #7235

Conversation

mmorhun commented Nov 7, 2017

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolusha Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinokurig left a comment

Choose a reason for hiding this comment

mmorhun commented Nov 9, 2017

codenvy-ci commented Nov 10, 2017

mmorhun commented Nov 10, 2017

mmorhun commented Nov 10, 2017

slemeur commented Nov 10, 2017

dmytro-ndp commented Nov 10, 2017

codenvy-ci commented Nov 10, 2017

mmorhun commented Nov 10, 2017

mmorhun commented Nov 10, 2017 • edited Loading

dmytro-ndp commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmytro-ndp Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmytro-ndp Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

mmorhun commented Nov 10, 2017 • edited Loading

mmorhun commented Nov 10, 2017

dmytro-ndp commented Nov 10, 2017 • edited Loading

tolusha Nov 7, 2017 •

edited

Loading

mmorhun commented Nov 10, 2017 •

edited

Loading

dmytro-ndp Nov 10, 2017 •

edited

Loading

dmytro-ndp Nov 13, 2017 •

edited

Loading

mmorhun commented Nov 10, 2017 •

edited

Loading

dmytro-ndp commented Nov 10, 2017 •

edited

Loading