Fixes #21643 - fix and move nfs_visibilty.js to webpack #5002

amirfefer · 2017-11-13T13:26:55Z

nfs section does not become visible when choosing a required nfs os family in installation media form.
This occurs due to broken script file - nfs_visibilty.js, which apparently doesn't work since moving to a select box from checkbox to choose an os family.
I re-wrote the logic and moved it to wepback

theforeman-bot · 2017-11-13T13:27:04Z

Issues: #21643

houndci-bot · 2017-11-13T13:27:04Z

app/helpers/mediums_helper.rb

+
+  def required_nfs_list
+    Hash[Operatingsystem.families.map {|key|
+      [key, key.constantize.require_nfs_access_to_medium]}].to_json


Expression at 6, 58 should be on its own line.

houndci-bot · 2017-11-13T13:27:04Z

app/helpers/mediums_helper.rb

@@ -1,3 +1,8 @@
 module MediumsHelper
  include PtablesHelper
+
+  def required_nfs_list
+    Hash[Operatingsystem.families.map {|key|


Avoid using {...} for multi-line blocks.

houndci-bot · 2017-11-13T13:27:04Z

app/helpers/mediums_helper.rb

+
+  def required_nfs_list
+    Hash[Operatingsystem.families.map {|key|
+      [key, key.constantize.require_nfs_access_to_medium]}].to_json


Expression at 6, 58 should be on its own line.

houndci-bot · 2017-11-13T13:27:04Z

app/helpers/mediums_helper.rb

@@ -1,3 +1,8 @@
 module MediumsHelper
  include PtablesHelper
+
+  def required_nfs_list
+    Hash[Operatingsystem.families.map {|key|


Avoid using {...} for multi-line blocks.

tbrisker

See comments inline.
Also, IIRC there was some dead css code you can get rid of while you're here.

tbrisker · 2017-11-14T08:19:44Z

webpack/assets/javascripts/foreman_medium.js

+      $('#nfs-section').show();
+    } else {
+      $('#nfs-section').hide();
+    }


you can make this a one liner:

$('#nfs-section').toggle(nfsRequired[osFamily.value]);

tbrisker · 2017-11-14T08:46:04Z

app/views/media/_form.html.erb

@@ -17,18 +16,15 @@
      <%= text_f f, :name %>
      <%= text_f f, :path, :size => "col-md-8", :help_block => _("The path to the medium, can be a URL or a valid NFS server (exclusive of the architecture).
          for example <em>http://mirror.centos.org/centos/$version/os/$arch</em> where <strong>$arch</strong> will be substituted for the host's actual OS architecture and <strong>$version</strong>, <strong>$major</strong> and <strong>$minor</strong> will be substituted for the version of the operating system. Solaris and Debian media may also use <strong>$release</strong>.").html_safe %>
-        <span id="nfs-section" <%= display?(!@medium.operatingsystems.map(&:require_nfs_access_to_medium).any?) %>>
+        <span id="nfs-section" <%= display?(!@medium.operatingsystems.map{ |obj| obj.class.require_nfs_access_to_medium }.any?) %>>


This won't work if the medium has solaris os_family but no operating systems assigned.
Why not also leverage the required_nfs_list helper here?
p.s. you can use any? with a block instead of .map{..}.any?

tbrisker · 2017-11-14T08:47:12Z

app/helpers/mediums_helper.rb

+    Hash[Operatingsystem.families.map do |key|
+      [key, key.constantize.require_nfs_access_to_medium]
+    end].to_json
+  end


Why not just return a list of all families requiring it and check if the selected family is in it or not?

amirfefer · 2017-11-15T15:54:57Z

@tbrisker , Thanks for your review, could you have a second look?

theforeman-bot · 2017-11-16T22:30:58Z

@amirfefer, this pull request is currently not mergeable. Please rebase against the develop branch and push again.

If you have a remote called 'upstream' that points to this repository, you can do this by running:

    $ git pull --rebase upstream develop

This message was auto-generated by Foreman's prprocessor

houndci-bot · 2017-11-16T22:34:14Z

webpack/assets/javascripts/foreman_medium.test.js

@@ -0,0 +1,31 @@
+jest.unmock('./foreman_medium');
+import $ from 'jquery';
+import {nfsVisibility} from './foreman_medium';


Import in body of module; reorder to top import/first
Expected empty line after import statement not followed by another import import/newline-after-import
A space is required after '{' object-curly-spacing
A space is required before '}' object-curly-spacing

houndci-bot · 2017-11-16T22:34:14Z

webpack/assets/javascripts/foreman_medium.test.js

@@ -0,0 +1,31 @@
+jest.unmock('./foreman_medium');
+import $ from 'jquery';


Import in body of module; reorder to top import/first

houndci-bot · 2017-11-16T22:34:14Z

webpack/assets/javascripts/foreman_medium.js

+import $ from 'jquery';
+
+export function nfsVisibility(osFamily, nfsRequired) {
+  nfsRequired.includes(osFamily.value) ? $('#nfs-section').show() : $('#nfs-section').hide();


Expected an assignment or function call and instead saw an expression no-unused-expressions

houndci-bot · 2017-11-16T22:34:14Z

webpack/assets/javascripts/bundle.js

@@ -34,4 +34,8 @@ window.tfm = Object.assign(window.tfm || {}, {
  reactMounter: require('./react_app/common/MountingService'),
  editor: require('./foreman_editor'),
  nav: require('./foreman_navigation'),
+<<<<<<< HEAD


Parsing error: Unexpected token

houndci-bot · 2017-11-16T22:35:47Z

webpack/assets/javascripts/bundle.js

@@ -34,4 +34,5 @@ window.tfm = Object.assign(window.tfm || {}, {
  reactMounter: require('./react_app/common/MountingService'),
  editor: require('./foreman_editor'),
  nav: require('./foreman_navigation'),
+  medium: require('./foreman_medium')


Missing trailing comma comma-dangle

tbrisker

Just one minor comment, other than that works well.

tbrisker · 2017-11-19T12:48:33Z

app/helpers/mediums_helper.rb

+
+  def required_nfs?
+    required_nfs_list.include?(@medium.os_family) ||
+    @medium.operatingsystems.any?{ |obj| obj.class.require_nfs_access_to_medium }


The second part of this check is redundant, you can only add operating systems that match the family

amirfefer · 2017-11-30T13:46:15Z

@tbrisker , Thanks for your review,
could you have a second look?

tbrisker

Looks good, will merge once tests pass.

tbrisker · 2017-11-30T22:33:06Z

Thanks @amirfefer !

houndci-bot reviewed Nov 13, 2017

View reviewed changes

theforeman-bot added Needs testing Not yet reviewed labels Nov 13, 2017

amirfefer force-pushed the 21643 branch from ae40a7f to d353920 Compare November 13, 2017 15:08

tbrisker requested changes Nov 14, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Nov 14, 2017

amirfefer force-pushed the 21643 branch from d353920 to 739b2eb Compare November 15, 2017 15:54

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 15, 2017

amirfefer force-pushed the 21643 branch 2 times, most recently from 818bc2f to 617eb77 Compare November 16, 2017 22:29

theforeman-bot removed Needs testing Needs re-review labels Nov 16, 2017

theforeman-bot added the Waiting on contributor label Nov 16, 2017

amirfefer force-pushed the 21643 branch from 617eb77 to 2e3b2a4 Compare November 16, 2017 22:34

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 16, 2017

houndci-bot reviewed Nov 16, 2017

View reviewed changes

amirfefer force-pushed the 21643 branch from 2e3b2a4 to 4fae6e7 Compare November 16, 2017 22:35

houndci-bot reviewed Nov 16, 2017

View reviewed changes

amirfefer force-pushed the 21643 branch from 4fae6e7 to 4a5a821 Compare November 16, 2017 23:04

tbrisker requested changes Nov 19, 2017

View reviewed changes

theforeman-bot removed the Needs re-review label Nov 19, 2017

theforeman-bot added the Waiting on contributor label Nov 19, 2017

Fixes #21643 - fix and move nfs_visibilty.js to webpack

a73992e

amirfefer force-pushed the 21643 branch from 4a5a821 to a73992e Compare November 30, 2017 13:44

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 30, 2017

tbrisker approved these changes Nov 30, 2017

View reviewed changes

tbrisker added Ready to merge and removed Needs re-review Needs testing labels Nov 30, 2017

tbrisker merged commit 8f7f710 into theforeman:develop Nov 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #21643 - fix and move nfs_visibilty.js to webpack #5002

Fixes #21643 - fix and move nfs_visibilty.js to webpack #5002

amirfefer commented Nov 13, 2017 •

edited

theforeman-bot commented Nov 13, 2017

houndci-bot Nov 13, 2017

houndci-bot Nov 13, 2017

houndci-bot Nov 13, 2017

houndci-bot Nov 13, 2017

tbrisker left a comment

tbrisker Nov 14, 2017

tbrisker Nov 14, 2017

tbrisker Nov 14, 2017

amirfefer commented Nov 15, 2017

theforeman-bot commented Nov 16, 2017

houndci-bot Nov 16, 2017

houndci-bot Nov 16, 2017

houndci-bot Nov 16, 2017

houndci-bot Nov 16, 2017

houndci-bot Nov 16, 2017

tbrisker left a comment •

edited

tbrisker Nov 19, 2017

amirfefer commented Nov 30, 2017

tbrisker left a comment

tbrisker commented Nov 30, 2017

		@@ -0,0 +1,31 @@
		jest.unmock('./foreman_medium');
		import $ from 'jquery';

Fixes #21643 - fix and move nfs_visibilty.js to webpack #5002

Fixes #21643 - fix and move nfs_visibilty.js to webpack #5002

Conversation

amirfefer commented Nov 13, 2017 • edited

theforeman-bot commented Nov 13, 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

tbrisker left a 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

amirfefer commented Nov 15, 2017

theforeman-bot commented Nov 16, 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

Choose a reason for hiding this comment

tbrisker left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Nov 30, 2017

tbrisker left a comment

Choose a reason for hiding this comment

tbrisker commented Nov 30, 2017

amirfefer commented Nov 13, 2017 •

edited

tbrisker left a comment •

edited