Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring back Git URL external access of the current project #1536

Merged
merged 1 commit into from Mar 26, 2018

Conversation

porcelli
Copy link
Member

this initial implementation aims to be a quick hack, enabling this long time request available for upcoming release.
It's expected to improve the UI visulation, bringing something like GitHub has.

@porcelli
Copy link
Member Author

jenkins full downstream build

Copy link

@jhrcek jhrcek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this locally and it works fine. Few comments inline. Also please add AF-1113 to the commit /PR title to make it easier to find in the future.

@@ -35,7 +35,7 @@ <h3 data-i18n-key="GeneralSettings" data-field="title"></h3>
</div>
<div class="form-group">
<label for="url" data-i18n-key="URL"></label>
<input type="text" class="form-control" id="url">
<input type="text" class="form-control" id="url" readonly>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By making this field read-only this PR is changing the semantics of the URL field in general settings.
Before this PR this field was connected to project pom.xml <url> element.
In this PR this has changed to dynamically display URL of the workbench-managed repository.

This means that we should remove this outdated functionality from GeneralSettings presenter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the semantic of the URL was always intented to be the one present in this PR, use the pom data wasn't the original intentention.

ps: I was part of the UI design and personally asked UX to add this field to cover exactly the Git URL. This was overlooked during the merge.

//this is basically for tests that don't use git file system
}

model.setGitUrl(value.replace("\n", " | "));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that getFilesystem().toString() is returning multiple lines with 1 url per line? I find that a bit confusing.. Could you please point me to some code where we're setting URL to contain newline-separated URLs? I'd like to understand this better.

Also it would be nice to have a unit test for this new loading behavior here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has always been the behavior of the jgit getFilesystem().toString(), here link for the code: https://github.com/kiegroup/appformer/blob/master/uberfire-nio2-backport/uberfire-nio2-impls/uberfire-nio2-jgit/src/main/java/org/uberfire/java/nio/fs/jgit/JGitFileSystemImpl.java#L104-L117

About the test: yes... ideally the test should cover this, but to make this happen this test has to be completely redone to use git not "file://" - and to do this it wouldn't be possible for the release happening today. I'd prefer to have this quick and dirty fix merged to be part of the community release and then follow up and fix the test.

… UX of workbench: display the git external access url's.

this initial implementation aims to be a quick hack, enabling this long time request available for upcoming release.
It's expected to improve the UI visulation, bringing something like GitHub has.
Copy link

@jhrcek jhrcek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this PR after discussion with @porcelli - the followup work (changing this quick impl to 2 readonly inputs with "copy to clip board" button, adding a test for this and removing the obsolete "set url in pom.xml") will be implemented later as part of AF-1113.

@porcelli porcelli merged commit 1d8ca9f into kiegroup:master Mar 26, 2018
kiereleaseuser pushed a commit that referenced this pull request Mar 26, 2018
… UX of workbench: display the git external access url's. (#1536)

this initial implementation aims to be a quick hack, enabling this long time request available for upcoming release.
It's expected to improve the UI visulation, bringing something like GitHub has.
(cherry picked from commit 1d8ca9f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants