-
Notifications
You must be signed in to change notification settings - Fork 287
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
Fixes #6842 - system uuid as id bz1122938 #4504
Conversation
@dustint-rh I think the commit needs the |
module Katello | ||
module Api | ||
module V2 | ||
module Presenter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation.
@ehelms, @daviddavis updated the summary and indentation. |
# | ||
# rubocop:disable TrivialAccessors | ||
module Katello | ||
module Api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dustint-rh the indentation still looks off I'm afraid. I know the old files do this but they aren't correct (they need to be fixed) and we typically indent the new files inside the Katello module.
Also, we typically put a blank line after the copyright at the top.
@daviddavis updated the indentation |
@@ -0,0 +1,18 @@ | |||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the module is presenters, should the directory also be named presenters
?
Why wouldn't we just do this in the Rabl? |
+1 to rabl. this change would still return the old system id for places using the rabl file and I think we don't want that. |
@ehelms, @daviddavis updated, using rabl instead |
attributes :name, :description | ||
node :id do |r| | ||
r.uuid | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just do:
attributes :uuid => :id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daviddavis not sure why, but that didn't seem to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? It seems to work for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's because :uuid has to also be listed as an attribute.
attributes :uuid => id, :uuid
this shows just {uuid: xyx }
attributes :uuid => id
this shows {id: xyz}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, that's weird. One thing you could do is make this one line:
node :id { |system| system.uuid }
APJ |
In the Api, display uuid as the id in both the system show and index actions.
[TEST] |
this is just a temporary fix, the api should only accept the db id and display the id as the database id |
[TEST] |
[test] |
Fixes #6842 - system uuid as id bz1122938
Presents the uuid as the id in both the system show and index actions.