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

UI: Adds network provisioning for Network provider #11442

Merged
merged 1 commit into from
Oct 7, 2016
Merged

UI: Adds network provisioning for Network provider #11442

merged 1 commit into from
Oct 7, 2016

Conversation

gildub
Copy link
Contributor

@gildub gildub commented Sep 22, 2016

Views: Added edit and new - using Angular (as cloud_volume)
Button: Adds configuration -> Add, Edit and Cancel
Controllers/Models:
EMS Network Provider updates using raw updates
to be replaced with tasks.

Includes rspec

  • Skips the "Edit tag" button

More fields to be added once following fixed:
fog/fog-openstack#192

@tzumainn
Copy link
Contributor

@miq-bot add_label euwe/yes

@miq-bot
Copy link
Member

miq-bot commented Sep 23, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@gildub
Copy link
Contributor Author

gildub commented Sep 29, 2016

@dclarizio, @martinpovolny, @himdel, could you please review?

if (cloudNetworkFormId == 'new') {
$scope.cloudNetworkModel.name = "";
$scope.newRecord = true;
$scope.cloudNetworkModel.enabled= true;
Copy link
Contributor

@himdel himdel Sep 30, 2016

Choose a reason for hiding this comment

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

Minor, but missing space before =..

EDIT: and same in the else branch..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks.


$scope.addClicked = function() {
miqService.sparkleOn();
var url = 'create/new' + '?button=add';
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to use the absolute (/cloud_network/create/new) url here? And why concatenate 2 constants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question and I honestly have no idea, it's the same in many other forms, included the cloud_volume ones I got inspired from.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aah, you're right .. OK, in that case, I don't care either way, I'll be doing a PR to fix the others anyway :).

$scope.saveClicked = function() {
miqService.sparkleOn();
var url = '/cloud_network/update/' + cloudNetworkFormId + '?button=save';
miqService.miqAjaxButton(url, true);
Copy link
Contributor

@himdel himdel Sep 30, 2016

Choose a reason for hiding this comment

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

Please don't use miqAjaxButton with true (trying to remove in #11324) - pass $scope.cloudNetworkModel instead (or an object with whatever data you want to send to the server in general).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, passing the scope model now.

@himdel
Copy link
Contributor

himdel commented Sep 30, 2016

@gildub I'm also seeing a lot of duplication between edit.html.haml and new.html.haml, any chance these could share a common partial?

@gildub
Copy link
Contributor Author

gildub commented Oct 3, 2016

@himdel, yes, absolutely!

@gildub
Copy link
Contributor Author

gildub commented Oct 4, 2016

@himdel, thanks for your help
@dclarizio, I think this one is ready

@himdel
Copy link
Contributor

himdel commented Oct 4, 2016

@gildub any time :) LGTM, except I still wonder about the duplication between new and edit partials. Why do they even have to be different? I do understand that network manager can only be set when adding (although it could still be visible and disabled, so that you can see the type in edit), but Provider Network Type and Cloud Tenant seem like there's no reason not to move them to the shared partial...

IMHO only the javascript part has to be different.

@gildub
Copy link
Contributor Author

gildub commented Oct 4, 2016

@himdel,

Actually once a network has been created there are fields that cannot be changed, the provider:network_type and tenant/project are among them.

When creating such as in new form those fields are displayed with a dropdown box.
When editing, those fields are displayed in plain only text.

Unless you have a mechanism to make the select read only I don't see any benefit to have those in a common partial where there is going to be a conditional to triage between a plain text or a select depending if it's edit or new.

@tzumainn
Copy link
Contributor

tzumainn commented Oct 5, 2016

👍 tested the UI; network was created, edited, and deleted correctly

@himdel
Copy link
Contributor

himdel commented Oct 5, 2016

@gildub Well, the mechanism is ng-readonly="newRecord", but.. I'll leave this one up to you. If you consider it more readable and maintainable to have it in two slightly different copies, I'll believe you :).

@miq-bot
Copy link
Member

miq-bot commented Oct 5, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@gildub gildub closed this Oct 6, 2016
@gildub gildub reopened this Oct 6, 2016
@gildub
Copy link
Contributor Author

gildub commented Oct 6, 2016

@himdel,

Ok for ng-readonly flag, but unfortunately that doesn't work for select as a whole. And even if it did that would mean provide a single value in case of update (readonly) which I think is messy. I suppose a more pure javascript approach would be possible but out of scope for now.

Views: Added edit and new - using Angular (as cloud_volume)

Button: Adds configuration -> Add, Edit and Cancel

Controllers/Models:
EMS Network Provider updates using raw updates
to be replaced with tasks.

Rspec:
  Couples of issues
@miq-bot
Copy link
Member

miq-bot commented Oct 7, 2016

Checked commit https://github.com/gildub/manageiq/commit/86475e9e2827ccf3ee7c62366d855a426e442da4 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
11 files checked, 28 offenses detected

app/controllers/cloud_network_controller.rb

app/helpers/application_helper/toolbar/cloud_network_center.rb

  • ❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

app/helpers/application_helper/toolbar/cloud_networks_center.rb

  • ❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

@dclarizio dclarizio merged commit 5c4d942 into ManageIQ:master Oct 7, 2016
@dclarizio dclarizio added this to the Sprint 48 Ending Oct 24, 2016 milestone Oct 7, 2016
@gildub gildub deleted the network-provisioning-network branch October 10, 2016 01:34
chessbyte pushed a commit that referenced this pull request Oct 13, 2016
UI: Adds network provisioning for Network provider
(cherry picked from commit 5c4d942)
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log
commit 3be35cc1e5ffb75378cb23089449d6d235609f56
Author: Dan Clarizio <dclarizi@redhat.com>
Date:   Fri Oct 7 13:48:46 2016 -0700

    Merge pull request #11442 from gildub/network-provisioning-network

    UI: Adds network provisioning for Network provider
    (cherry picked from commit 5c4d9424ec2be2fd2171aac313d63bd5fff9b360)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants