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

CLDIDE-2631: Change behavior for perClick policy, add error notificat… #451

Merged
merged 2 commits into from
Nov 27, 2015

Conversation

akorneta
Copy link
Contributor

…ions

@@ -124,7 +125,7 @@ public void tryStartWorkspace() {
final WorkspaceConfigDto workspaceConfigDto = factory.getWorkspace();

if (workspaceConfigDto == null) {
//TODO handle this situation
notificationManager.showError("Workspace config is not defined");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add return here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it needed here, thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

Also move please message to localization contant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@mshaposhnik
Copy link
Contributor

Ok

@akorneta akorneta force-pushed the CLDIDE-2631 branch 2 times, most recently from 053dea5 to cc70bfe Compare November 26, 2015 15:26
@skabashnyuk
Copy link
Contributor

If workspace already exists You should add counter to the name

@@ -470,6 +470,9 @@
@Key("workspace.start.failed")
String workspaceStartFailed(String workspaceName);

@Key("workspace.start.failed")
Copy link
Contributor

Choose a reason for hiding this comment

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

Change please key for this property. I think It'll broke gwt compiling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@akorneta akorneta force-pushed the CLDIDE-2631 branch 2 times, most recently from 35fcaf3 to de23be3 Compare November 27, 2015 09:07
*/
private Promise<UsersWorkspaceDto> getWorkspaceByConditionOrCreateNew(final WorkspaceConfigDto workspaceConfigDto,
final Function<UsersWorkspaceDto, Boolean> condition) {
final Function<UsersWorkspaceDto, Boolean> condition,
final boolean generateName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to name this parameter something like useExists or createNewWorkspace

Copy link
Contributor

Choose a reason for hiding this comment

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

or reuseExisted
wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@sleshchenko
Copy link
Contributor

Other OK for me

1 similar comment
@skabashnyuk
Copy link
Contributor

Other OK for me

akorneta added a commit that referenced this pull request Nov 27, 2015
CLDIDE-2631: Change behavior for perClick policy, add error notificat…
@akorneta akorneta merged commit 98f6f2a into 4.0 Nov 27, 2015
@akorneta akorneta deleted the CLDIDE-2631 branch November 27, 2015 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants