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

Use properties in the model when retrieving the ansible repo for editing #1230

Conversation

lgalis
Copy link
Contributor

@lgalis lgalis commented May 3, 2017

Only copy the properties in the model when retrieving the Ansible repository info for editing

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

  • fixes the extra attributes sent to the tower gem for repository edit

/cc @jameswnl
/cc @ZitaNemeckova

@miq-bot miq-bot added the wip label May 3, 2017
@lgalis lgalis force-pushed the use_only_repository_properties_in_model_on_edit branch from 8a1d397 to 5c11830 Compare May 3, 2017 00:16
@lgalis lgalis changed the title [WIP] Use properties in the model when retrieving the ansible repo for editing Use properties in the model when retrieving the ansible repo for editing May 3, 2017
@lgalis
Copy link
Contributor Author

lgalis commented May 3, 2017

@miq-bot add_label bug, fine/yes, euwe/no

@miq-bot miq-bot added bug euwe/no and removed wip labels May 3, 2017
@lgalis lgalis changed the title Use properties in the model when retrieving the ansible repo for editing [WIP]Use properties in the model when retrieving the ansible repo for editing May 3, 2017
@miq-bot miq-bot added the wip label May 3, 2017
@lgalis lgalis changed the title [WIP]Use properties in the model when retrieving the ansible repo for editing Use properties in the model when retrieving the ansible repo for editing May 3, 2017
@lgalis
Copy link
Contributor Author

lgalis commented May 3, 2017

@miq-bot add_label blocker

@lgalis
Copy link
Contributor Author

lgalis commented May 3, 2017

@h-kataria or @dclarizio - please review

@miq-bot miq-bot added blocker and removed wip labels May 3, 2017
@himdel
Copy link
Contributor

himdel commented May 3, 2017

@lgalis This is moving us in completely the opposite direction.. In general we should be able to save everything we got from the server.

So.. what is this about? What specifically are the fields that appear in the model and should not be there when saving?

EDIT: if this is about managerResource, then I guess you should just be doing delete vm.repositoryModel.managerResource in saveClicked.

@lgalis
Copy link
Contributor Author

lgalis commented May 3, 2017

@himdel - no - this is about href and actions that we are saving in the angular model ( and then sending on update )when getting the data from the API
This PR is to read only what is in the angular model - as these are the attributes we will be updating,
The alternative is to remove what we know is not part of the actual data - like the href and actions.

@himdel
Copy link
Contributor

himdel commented May 3, 2017

actions and href

Wow, that may need some more discussion with the API people, seems like those should never be mixed together with the record data.. but for now, yeah, I still prefer the blacklist approach over a whitelist, unless it's a problem somehow..

@himdel
Copy link
Contributor

himdel commented May 3, 2017

.. ok, thinking about this some more, this exact same problem happens for every single API endpoint.

So.. I guess we should just drop this fix, and fix it on the API side, because otherwise you need to fix every single API user in the UI to delete those 2 attributes.

@jameswnl Laura mentioned you have a fix for that on the API side? Is there a PR please?

(The issue that actions and href should be ignored from any POST & PUT & PATCH to the API)

@lgalis
Copy link
Contributor Author

lgalis commented May 3, 2017

@himdel - not for the API, for the client - ansible/ansible_tower_client_ruby#93
I will discuss with API team

@jameswnl
Copy link
Contributor

jameswnl commented May 3, 2017

Thanks Laura, you are fast.

Yes, it's on the client gem.

@himdel
Copy link
Contributor

himdel commented May 3, 2017

Oh, but then we definitely need an API fix. Or, we could filter it in the common code in miq_api.js for now, but .. I'd much rather see this fixed on the API side...

@lgalis
Copy link
Contributor Author

lgalis commented May 3, 2017

An API fix is not required nor would it be enough here, There are many fields that we do not need to retrieve.
I will be modifying the PR to only request from the API the fields we need.
This approach can be discussed with our team and the API team and modified as needed but not in the scope of this PR.

@dclarizio
Copy link

@lgalis looks like an easy cc fix

@himdel

I will be modifying the PR to only request from the API the fields we need.

@lgalis and I discussed fetching only the fields needed, similar to what we've seen in the SUI, so hopefully that approach looks good to you. Please re-review as we're hoping to get this in for the blocker BZ.

@lgalis lgalis force-pushed the use_only_repository_properties_in_model_on_edit branch from b6e620c to c35c8fb Compare May 4, 2017 01:11
@lgalis lgalis force-pushed the use_only_repository_properties_in_model_on_edit branch from c35c8fb to 671fe00 Compare May 4, 2017 01:23
@miq-bot
Copy link
Member

miq-bot commented May 4, 2017

Checked commits lgalis/manageiq-ui-classic@5c11830~...671fe00 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@himdel
Copy link
Contributor

himdel commented May 4, 2017

Thanks, well, I definitely like the delete approach more, but sending the list of attributes is just another whitelist.. but never mind, no real objections now :).

@dclarizio dclarizio merged commit 5bc21d4 into ManageIQ:master May 4, 2017
@dclarizio dclarizio added this to the Sprint 60 Ending May 8, 2017 milestone May 4, 2017
@lgalis lgalis deleted the use_only_repository_properties_in_model_on_edit branch May 4, 2017 14:05
simaishi pushed a commit that referenced this pull request May 4, 2017
…n_model_on_edit

Use properties in the model when retrieving the ansible repo for editing
(cherry picked from commit 5bc21d4)

https://bugzilla.redhat.com/show_bug.cgi?id=1448098
@simaishi
Copy link
Contributor

simaishi commented May 4, 2017

Fine backport details:

$ git log -1
commit c8afdc762d7dda4e0cf4f708c07e985f7c08c848
Author: Dan Clarizio <dclarizi@redhat.com>
Date:   Thu May 4 06:40:12 2017 -0700

    Merge pull request #1230 from lgalis/use_only_repository_properties_in_model_on_edit
    
    Use properties in the model when retrieving the ansible repo for editing
    (cherry picked from commit 5bc21d4ea0e07ed4e06b0d7bdad14fb8f64606db)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1448098

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

6 participants