fixes #4768 - various activation key page issues #3901
Conversation
jlsherrill
commented
Mar 27, 2014
jlsherrill
commented
- Default content View was not selectable (fixed across app)
- Using a paged API for content view dropdown (fixed across the app)
- Fixes environment selector on new activation key page, no disable option needed
- Updated text slightly on activation key page to make it more user friendly
|
Tested out locally and below are some observations:
I realize these are not related to the changes in this PR; therefore, perhaps we can address them separately. |
|
NOTE: We can now select Library+Default Organization view when creating an akey, so +1:) |
|
ACK pending comments; however, redmine issues will be fine as well. |
|
@bbuckingham I'll fix #3 and do issues for the rest, thanks! |
|
@jlsherrill,Sounds good, thx! |
| @@ -28,7 +28,8 @@ angular.module('Bastion.content-views').factory('ContentView', | |||
| return $resource('/api/v2/content_views/:id/:action', | |||
| {id: '@id', 'organization_id': CurrentOrganization}, | |||
| { | |||
| query: {method: 'GET', isArray: false, params: {nondefault: true}}, | |||
| query: {method: 'GET', isArray: false}, | |||
| nonPaged: {method: 'GET', isArray: false, params: {'full_result': true}}, | |||
There was a problem hiding this comment.
What do you see as the benefits of adding a new method as opposed to just passing {'full_result': true} when needed? Are we always going to want all the results? If so why not just change query to pass {'full_result': true}?
There was a problem hiding this comment.
I tend to agree with @waldenraines with respect to just passing the parameter when you need to instead of creating a new method.
There was a problem hiding this comment.
I started down that route initially but ended up adding it in 5 placesIIRC. It had been used everywhere for every single drop down. When thinking about how to keep this from occurring in the future a different action seemed more ideal than the params (not that either is fool poof).
There was a problem hiding this comment.
@ehelms @waldenraines thoughts?
There was a problem hiding this comment.
@jlsherrill params tell a function how to configure itself which is sort of what you are doing here. I don't directly see what makes specifying 'full_result' different than specifying 'environment_id'. There's an inconsistency there in terms of what qualifies for it's own method and what doesn't to me.
There was a problem hiding this comment.
@waldenraines We're already adding query everywhere correct?
[jlsherri@centos bastion]$ grep query app/assets/javascripts/ -R | grep factory | wc -l
26
[jlsherri@centos bastion]$ find app/assets/javascripts/ | grep factory | wc -l
26
looks like it.
There was a problem hiding this comment.
Ah, good point, you're right we are because of isArray: false since it returns an object.
Okay, I'm fine with going with queryPaged and queryUnpaged then.
There was a problem hiding this comment.
@ehelms good with queryPaged and queryUnpaged ?
There was a problem hiding this comment.
There was a problem hiding this comment.
Good with queryPaged and queryUnpaged because of the reasons stated (the fact that we already define query explicitly).
|
These two issues may be related to this and should either be marked as duplicates or added to the list of what's fixed in the commit message: http://projects.theforeman.org/issues/4719 |
|
http://projects.theforeman.org/issues/4813 is not related at all and is related to showing the default CV in the CV list (see the first comment in the issue). |
|
@bbuckingham actually i will file issues for all the rest |
|
tested & ack |
|
ACK from me too. |
| <span class="help-block" ng-show="activationKey.environment !== undefined && contentViews.length === 0" translate> | ||
| The selected environment contains no Content Views, please select a different environment. | ||
| </span> | ||
| <span class="help-block" ng-show="activationKey.environment == undefined" translate> |
There was a problem hiding this comment.
=== for consistency and proper evaluation here.
|
APJ |
* Default content View was not selectable * Using a paged API for content view dropdown (fixed across the app) * Fixes environment selector on new activation key page, no disable option needed * Updated text slightly on activation key page to make it more user friendly
fixes #4768 - various activation key page issues
