Bug 1505782 - Environment From Fix Drag & Order Display #2238

cdcabrera · 2017-10-09T18:54:25Z

Fix for making sure the drag and drop/order handles are hidden within a read only display. Related to #2182

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1505782

cdcabrera · 2017-10-09T19:04:26Z

app/views/directives/edit-environment-from.html

@@ -52,7 +52,7 @@
        </div>
      </div>

-      <div ng-if="!$ctrl.isReadonlyAny && !entry.isReadonlyValue" class="environment-from-editor-button">
+      <div ng-if="$ctrl.isEnvFromSelectable()" class="environment-from-editor-button">


Going to look at placing the isReadonlyAny and isReadonlyValue back in...

Separated out the options length check and encased both isReadonlyAny and isReadonlyValue within the isEnvFromReadonly method

spadgett · 2017-10-09T19:41:41Z

app/scripts/directives/editEnvironmentFrom.js

-        entry.isReadonlyValue === true ||
-        ((entry.secretRef || entry.configMapRef) && !entry.selectedEnvFrom) ||
-        _.isEmpty(ctrl.envFromSelectorOptions);
+      return ctrl.isReadonlyAny || entry.isReadonlyValue === true || ((entry.secretRef || entry.configMapRef) && !entry.selectedEnvFrom);


We could simplify this a lot by just saying the whole thing is either read-only or it isn't. We currently don't have a need to make individual values read-only.

That works, removing the "isReadonlyAny" and tweaking some of the related logic

spadgett · 2017-10-09T19:42:27Z

app/scripts/directives/editEnvironmentFrom.js

@@ -50,11 +50,12 @@
      ctrl.editEnvironmentFromForm.$setDirty();
    };

+    ctrl.isEnvFromSelectable = function() {


I suggest hasOptions

spadgett · 2017-10-10T14:21:12Z

app/scripts/directives/editEnvironmentFrom.js

-        entry.isReadonlyValue === true ||
-        ((entry.secretRef || entry.configMapRef) && !entry.selectedEnvFrom) ||
-        _.isEmpty(ctrl.envFromSelectorOptions);
+      return entry.isReadonlyValue === true || ((entry.secretRef || entry.configMapRef) && !entry.selectedEnvFrom);


I was proposing getting rid of isReadonlyValue and keeping only isReadonlyAny :)

I don't think we have a need for only certain values being read only. Either the entire list is read only or it isn't

Hahahaha whoops, I'll go the other direction...

Edit:

To remove some of the confusion around this considering a ~~going to~~ rename for the value to towards "isReadOnly".

The current Read Only behavior removes the related binding parameter/property that can be passed in individually, and relies only on the Options List being populated and a Can I check for Secrets and Config Maps.

After trying to pick this apart looks like the piece around Secrets or Config maps and duplicate entries falls into a strange area. Specifically around when a user edits the Yaml and purposefully enters duplicates.

With the current version these duplicates become read only because they lack the additional property around "selectedEnvFrom" due to the way lodash and find is being used... this is unintended but has kinda given us an inadvertent solution to the duplicate issue. Similar to the below example...

I'm not sure I understand why we made duplicates read only, or missing secrets for that matter. Why prevent the user from editing those to fix the problem?

I think it should be a warning if it's there twice, but we shouldn't block it since it's not a validation error.

Concur on the blocking at all, that shouldn't happen, and doesn't... kinda. The current implementation doesn't block a user from deleting or rearranging an item, just creating an additional duplicate/triplicate/quad/etc entry through the interface.

Can and will look at using lodash filter instead of find to handle the checks, this may correct the problem, keep you posted

@spadgett looks like the aiming towards _.filter achieved the desired result, no more entries being read only if they're duplicates. Leveraged the same concept to apply a temporary placeholder duplicate class around the individual UI-Selects

@spadgett & @benjaminapetersen

On the Readonly state as an all or nothing piece... can push them up, just need a confirmation for the conditions on my local, which so far encompass...

If options aren't populated, everything is Read Only

If either the Secrets OR Config Maps "Can I" checks fail, everything is Read Only

And finally, removed the block (similar to below) for binding attribute pass-through for Read Only
if('isReadonly' in $attrs) { ctrl.isReadonlyAny = true; }

spadgett · 2017-10-13T14:18:03Z

/test

benjaminapetersen

Just a couple optional nits, but works for me when tested. I still do see that flicker after edits, but that might be addressed in your other PR.

benjaminapetersen · 2017-10-17T17:46:32Z

app/scripts/directives/editEnvironmentFrom.js

@@ -99,8 +100,23 @@
      });
    };

-    var updateEnvFromEntries = function(entries) {
-      ctrl.envFromEntries = entries || [];
+    var getReferenceValues = function(option) {


You might be able to nix the switch statements & eliminate some duplication with:

var getReferenceValues = function(option) { var kindRef = _.camelCase(option.kind) + 'Ref'; return _.filter(ctrl.envFromEntries, [kindRef, 'name', option.metadata.name]); };

(untested)

Prob similar could be done to trim down lines 76-91

@benjaminapetersen looks like coincidentally wiped some of the duplication out. Good catch on the switch

spadgett · 2017-10-19T18:50:39Z

app/scripts/directives/editEnvironmentFrom.js

+    };
+
+    ctrl.hasEntries = function() {
+      return angular.toJson(ctrl.entries) !== '[{}]' && ctrl.entries && ctrl.entries.length >= 1;


return _.some(ctrl.entries, function(entry) { return _.get(entry, 'configMapRef.name') || _.get(entry, 'secretRef.name'); });

spadgett · 2017-10-19T18:59:57Z

app/scripts/directives/editEnvironmentFrom.js

-          break;
-      }
+      _.each(ctrl.envFromSelectorOptions, function(option) {
+        _.each(getReferenceValues(option), function(val, i) {


You should create two maps of options secretsByName and configMapsByName when options change. Then you can index directly into the map using configMapsByName[option.configMapRef.name], etc.

spadgett

Need separate maps for secrets and config maps since name is not unique.

I suggest this warning style for duplicates:

spadgett · 2017-10-20T19:11:22Z

app/scripts/directives/editEnvironmentFrom.js


      if(!ctrl.envFromEntries.length) {
        addEntry(ctrl.envFromEntries);
      }
+      _.each(ctrl.envFromSelectorOptions, function(option) {
+        optionsByName[option.metadata.name] = option;
+      });


Select with .has-warning and .help-block

Fix for making sure the drag and order handles are hidden within a read only display

spadgett · 2017-10-24T18:08:54Z

@cdcabrera The form is showing up as read only for me when it should be editable:

spadgett · 2017-10-24T18:37:35Z

I can no longer reproduce the readonly problem.

spadgett · 2017-10-24T18:42:56Z

/lgtm
/hold

Needs #2367 to go in first or merge will fail

spadgett · 2017-10-24T19:07:38Z

/hold cancel

openshift-merge-robot · 2017-10-24T19:31:58Z

Automatic merge from submit-queue.

cdcabrera requested a review from spadgett October 9, 2017 18:54

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 9, 2017

cdcabrera commented Oct 9, 2017

View reviewed changes

cdcabrera force-pushed the issue-envfrom-readonly branch from 03a5544 to d7a7b6c Compare October 9, 2017 19:31

cdcabrera changed the title ~~[WIP] Environment From Fix Drag & Order Display~~ Environment From Fix Drag & Order Display Oct 9, 2017

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 9, 2017

spadgett reviewed Oct 9, 2017

View reviewed changes

cdcabrera force-pushed the issue-envfrom-readonly branch from d7a7b6c to cda77e6 Compare October 9, 2017 20:03

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 9, 2017

cdcabrera requested a review from benjaminapetersen October 9, 2017 20:35

spadgett reviewed Oct 10, 2017

View reviewed changes

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 11, 2017

benjaminapetersen reviewed Oct 17, 2017

View reviewed changes

cdcabrera force-pushed the issue-envfrom-readonly branch 2 times, most recently from 963e6d0 to c5e53dd Compare October 17, 2017 20:12

cdcabrera mentioned this pull request Oct 18, 2017

EnvFrom Config Map and Secret Link #2201

Merged

spadgett requested changes Oct 19, 2017

View reviewed changes

cdcabrera force-pushed the issue-envfrom-readonly branch from c5e53dd to 2078a33 Compare October 20, 2017 13:27

spadgett requested changes Oct 20, 2017

View reviewed changes

cdcabrera force-pushed the issue-envfrom-readonly branch 2 times, most recently from b5f4b72 to bdf9017 Compare October 20, 2017 22:20

Environment From Fix Drag & Order Display

9dc660c

Fix for making sure the drag and order handles are hidden within a read only display

cdcabrera force-pushed the issue-envfrom-readonly branch from bdf9017 to 9dc660c Compare October 24, 2017 15:59

openshift-ci-robot assigned spadgett Oct 24, 2017

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Oct 24, 2017

spadgett changed the title ~~Environment From Fix Drag & Order Display~~ Bug 1505782 - Environment From Fix Drag & Order Display Oct 24, 2017

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2017

openshift-merge-robot merged commit 849dda4 into openshift:master Oct 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1505782 - Environment From Fix Drag & Order Display #2238

Bug 1505782 - Environment From Fix Drag & Order Display #2238

cdcabrera commented Oct 9, 2017 •

edited by spadgett

Loading

cdcabrera Oct 9, 2017 •

edited

Loading

cdcabrera Oct 9, 2017

spadgett Oct 9, 2017

cdcabrera Oct 9, 2017

spadgett Oct 9, 2017

spadgett Oct 10, 2017

cdcabrera Oct 10, 2017 •

edited

Loading

cdcabrera Oct 10, 2017 •

edited

Loading

spadgett Oct 10, 2017

cdcabrera Oct 11, 2017

cdcabrera Oct 11, 2017

spadgett commented Oct 13, 2017

benjaminapetersen left a comment •

edited

Loading

benjaminapetersen Oct 17, 2017

benjaminapetersen Oct 17, 2017

cdcabrera Oct 17, 2017

spadgett Oct 19, 2017

spadgett Oct 19, 2017

spadgett left a comment

spadgett Oct 20, 2017

sg00dwin Oct 20, 2017

spadgett commented Oct 24, 2017

spadgett commented Oct 24, 2017

spadgett commented Oct 24, 2017

spadgett commented Oct 24, 2017

openshift-merge-robot commented Oct 24, 2017

Bug 1505782 - Environment From Fix Drag & Order Display #2238

Bug 1505782 - Environment From Fix Drag & Order Display #2238

Conversation

cdcabrera commented Oct 9, 2017 • edited by spadgett Loading

cdcabrera Oct 9, 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

cdcabrera Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

cdcabrera Oct 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

spadgett commented Oct 13, 2017

benjaminapetersen left a comment • 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

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Oct 24, 2017

spadgett commented Oct 24, 2017

spadgett commented Oct 24, 2017

spadgett commented Oct 24, 2017

openshift-merge-robot commented Oct 24, 2017

cdcabrera commented Oct 9, 2017 •

edited by spadgett

Loading

cdcabrera Oct 9, 2017 •

edited

Loading

cdcabrera Oct 10, 2017 •

edited

Loading

cdcabrera Oct 10, 2017 •

edited

Loading

benjaminapetersen left a comment •

edited

Loading