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.

Sign up for GitHub

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

Jump to bottom

ISPN-6779 Improve Security Configuration screen #115

Closed
wants to merge 4 commits into from
Closed

ISPN-6779 Improve Security Configuration screen #115

wants to merge 4 commits into from

Conversation

vblagoje
Copy link

@vblagoje vblagoje commented Jul 19, 2016 •
edited

Master only. Do not integrate yet. Feedback and review, please @ryanemerson

ryanemerson
ryanemerson reviewed Jul 25, 2016
View reviewed changes
src/main/webapp/components/directives/cache/security/security.js
};

scope.isAnyFieldModified = function () {
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this always be true?

@ryanemerson
Copy link
Collaborator

@vblagoje A few thoughts:

  1. As we are now using it's own directive, and such features are possible, should we disable the selecting of roles if security is not currently enabled?
  2. As we provide an 'undo change' operation throughout the cache config screens, should we not do the same here? My thoughts are that we should ideally allow this for the role selectors, but at the very least it should be there for the 'enable' checkbox.

@vblagoje
Copy link
Author

@ryanemerson done on both points. Please review again, squash and integrate!

ryanemerson
ryanemerson reviewed Jul 28, 2016
View reviewed changes
src/main/webapp/components/directives/cache/security/security.js

var module = angular.module('ispn.directives.cache.security', ['ispn.services.utils']);

module.directive('security', ['utils', '$modal', function (utils, modal) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

$modal is redundant

@ryanemerson
Copy link
Collaborator

@vblagoje Integrated ed88e77, thanks.

@vblagoje vblagoje deleted the t_6779 branch January 17, 2018 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ryanemerson ryanemerson

Labels
Ready for Review
Projects
None yet
Milestone
No milestone
2 participants
@vblagoje @ryanemerson