Fixes #15621 - WebUI: Cloned roles saved with nil builtin #3630

vision2910 · 2016-07-08T11:08:20Z

The above fix will prevent nil value for 'builtin' attribute for Role while cloning it. The changed files are-

/app/controllers/roles_controller.rb#L84

/test/functional/roles_controller_test.rb#L83

theforeman-bot · 2016-07-08T11:20:37Z

There were the following issues with the commit message:

length of the first commit message line for 6c74f37 exceeds 65 characters
commit message for 6c74f37 is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

dLobatog · 2016-07-11T08:05:12Z

@vision2910 Sorry but I can't reproduce this one. Check out this gif trying to reproduce it: I clone a builtin role, and it's properly displayed as not builtin. In the API (sorry I cropped the picture too much) it returns a 0!

Is that what you're seeing? If not, check out the latest branch (develop) of this repo and try to reproduce it there. Thank you!

vision2910 · 2016-07-11T09:24:20Z

@dLobatog thanks for reviewing the changes. I am also not able to reproduce it in the latest branch code(develop).

But in the file /app/controllers/roles_controller.rb#L84, we are assigning 'false' to the 'builtin' attribute while it is an integer attribute. Down the line it may cause some issue. Suggest if we can set it to integer zero instead of false value?

dLobatog · 2016-07-11T11:06:57Z

@vision2910 From what I can tell, that's not really a problem as it's saved as 0 with false. I agree it's not ideal, and it all stems from the fact the builtin field is an integer instead of a boolean (but it's never treated as anything other than a boolean from what I can see).

If you can write a db migration to change the field to be boolean in the db, I think that'd be ideal and we could merge that. Thank you!

vision2910 · 2016-07-11T11:43:14Z

@dLobatog point noted, will do the db changes.
Thanks!

vision2910 · 2016-07-12T05:05:25Z

@dLobatog missed to mention that 'builtin' can have value
0-2 (https://github.com/theforeman/foreman/blob/develop/app/models/role.rb#L53), so it cant be boolean.

dLobatog · 2016-07-12T09:25:38Z

@vision2910 Good point, thanks. Let's trigger tests with [test] & I'll merge if green.

domcleal · 2016-07-12T09:26:54Z

The value of 1 for builtin is no longer possible since 4a4c48e removed the default user role, so the two values of 0 and 2 could now be mapped to a boolean, assuming no further built-in roles are added in future.

vision2910 · 2016-07-14T05:16:27Z

Thanks @domcleal for pointing out the accurate possible values for builtin column.
I think it will be good to change the builtin from integer to boolean type. @dLobatog I will write a migration for that and trigger the test.

Thanks

vision2910 · 2016-07-14T09:16:47Z

@dLobatog @domcleal
I have analysed the code & feel that if we change the role.builtin's type from integer to boolean and store 0, 2 in it, in future it may cause confusion for other contributors because it will store non boolean value in role.builtin.

To avoid this confusion here are the below actions we can take-
1- Change the builtin role to 1 instead of 2 in-
https://github.com/theforeman/foreman/blob/develop/app/models/role.rb#L26
2- Change the validation to accomodate only 0, 1 as a value in- https://github.com/theforeman/foreman/blob/develop/app/models/role.rb#L53
3- Write a migration to change the role.builtin data type to boolean
4- Write a migration to update the value of existing role.builtin to 1 instead of 2.
5- Write the test cases to cover the above scenario.

Let me know your comments in this regard...
Thanks

domcleal · 2016-07-18T13:18:51Z

Well, I don't think moving the specific numbers around is necessary if the field is simply changed to a boolean. Writing a DB migration that migrates the ints safely to a boolean field is probably all that is required.

This may require some additional steps (such as creating, then renaming a temporary boolean column) if conversion isn't possible on all DBs in a single statement.

theforeman-bot · 2016-07-19T08:02:39Z

There were the following issues with the commit message:

length of the first commit message line for 495556d exceeds 65 characters

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

vision2910 · 2016-07-19T09:54:56Z

@domcleal I have verified that storing anything other than 0,1 or true, false in a boolean attribute would cause the problem. We have to avoid using 2 as a role.builtin's value.

dLobatog · 2016-07-19T10:41:17Z

[test]

ares · 2016-07-21T08:03:27Z

db/migrate/20160718114501_update_existing_builtin_to_boolean1_in_roles.rb

+    roles = Role.where(:builtin => 2)
+    roles.each do |role|
+      role.builtin = 1
+      role.save!


this should either use fake model or some AR method to skip validations to avoid problems in future, please see example in some older migration

SQL would be better here.
ActiveRecord::Base.connection.execute("UPDATE roles SET builtin = 1 WHERE builtin = 2")

ares · 2016-07-21T08:06:49Z

test/unit/role_test.rb

@@ -63,6 +63,16 @@ class RoleTest < ActiveSupport::TestCase
        assert_equal Role::BUILTIN_DEFAULT_ROLE, role.builtin
      end
    end
+
+    should "have zero or one builtin" do
+      role = Role.new(:name => 'role name')


this should be defined above the should block as

let(:role) { FactoryGirl.build(:role) }

ares · 2016-07-21T08:12:22Z

I think a lot of tests fail because factory for role should be updated too

ares · 2016-08-04T13:24:46Z

Btw the reason it works in develop branch is that recently merged PR fixed default value of builtin attribute https://github.com/theforeman/foreman/pull/3659/files#diff-dbdb4ceb4a2d790f9001a3bc2be67da1R3

theforeman-bot · 2016-10-08T15:44:25Z

There were the following issues with the commit message:

length of the first commit message line for 495556d exceeds 65 characters

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2016-10-08T15:44:25Z

Can an existing organization member please verify this patch?

theforeman-bot · 2016-10-08T15:44:25Z

Can an existing organization member please verify this patch?

theforeman-bot · 2016-10-08T15:44:26Z

Can an existing organization member please verify this patch?

theforeman-bot · 2017-04-13T07:29:10Z

Thank you for your contribution, @vision2910! This PR has been inactive for 6 months, closing for now.
Feel free to reopen when you return to it. This is an automated process.

vision2910 force-pushed the 1350354-WebUI-cloned-role-fix branch from 7cec956 to 6c74f37 Compare July 8, 2016 11:18

domcleal changed the title ~~Fixes #1350354 - WebUI: Cloned roles saved with nil builtin~~ Fixes #15621 - WebUI: Cloned roles saved with nil builtin Jul 8, 2016

theforeman-bot added the Waiting on contributor label Jul 8, 2016

domcleal added Not yet reviewed Needs testing labels Jul 8, 2016

dLobatog removed Needs testing Not yet reviewed labels Jul 11, 2016

Fixes #15621 - Cloned roles saved with nil value for builtin attribute

495556d

vision2910 force-pushed the 1350354-WebUI-cloned-role-fix branch from 6c74f37 to 495556d Compare July 19, 2016 08:02

theforeman-bot added Needs re-review Needs testing Waiting on contributor and removed Waiting on contributor labels Jul 19, 2016

ares reviewed Jul 21, 2016
View reviewed changes

ares removed Needs re-review Needs testing labels Jul 21, 2016

ranjan mentioned this pull request Oct 8, 2016

Fixes #16828 - cloned-roles - role.builtin must be 0 #3925

Merged

theforeman-bot added the Waiting on contributor label Oct 8, 2016

theforeman-bot added the Inactive label Apr 13, 2017

theforeman-bot closed this Apr 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #15621 - WebUI: Cloned roles saved with nil builtin #3630

Fixes #15621 - WebUI: Cloned roles saved with nil builtin #3630

vision2910 commented Jul 8, 2016

theforeman-bot commented Jul 8, 2016

dLobatog commented Jul 11, 2016

vision2910 commented Jul 11, 2016

dLobatog commented Jul 11, 2016

vision2910 commented Jul 11, 2016

vision2910 commented Jul 12, 2016

dLobatog commented Jul 12, 2016

domcleal commented Jul 12, 2016

vision2910 commented Jul 14, 2016

vision2910 commented Jul 14, 2016 •

edited

Loading

domcleal commented Jul 18, 2016

theforeman-bot commented Jul 19, 2016

vision2910 commented Jul 19, 2016

dLobatog commented Jul 19, 2016

ares Jul 21, 2016

swapab Oct 8, 2016 •

edited

Loading

ares Jul 21, 2016

ares commented Jul 21, 2016

ares commented Aug 4, 2016

theforeman-bot commented Oct 8, 2016

theforeman-bot commented Oct 8, 2016

theforeman-bot commented Oct 8, 2016

theforeman-bot commented Oct 8, 2016

theforeman-bot commented Apr 13, 2017

Fixes #15621 - WebUI: Cloned roles saved with nil builtin #3630

Fixes #15621 - WebUI: Cloned roles saved with nil builtin #3630

Conversation

vision2910 commented Jul 8, 2016

theforeman-bot commented Jul 8, 2016

dLobatog commented Jul 11, 2016

vision2910 commented Jul 11, 2016

dLobatog commented Jul 11, 2016

vision2910 commented Jul 11, 2016

vision2910 commented Jul 12, 2016

dLobatog commented Jul 12, 2016

domcleal commented Jul 12, 2016

vision2910 commented Jul 14, 2016

vision2910 commented Jul 14, 2016 • edited Loading

domcleal commented Jul 18, 2016

theforeman-bot commented Jul 19, 2016

vision2910 commented Jul 19, 2016

dLobatog commented Jul 19, 2016

ares Jul 21, 2016

Choose a reason for hiding this comment

swapab Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

ares Jul 21, 2016

Choose a reason for hiding this comment

ares commented Jul 21, 2016

ares commented Aug 4, 2016

theforeman-bot commented Oct 8, 2016

theforeman-bot commented Oct 8, 2016

theforeman-bot commented Oct 8, 2016

theforeman-bot commented Oct 8, 2016

theforeman-bot commented Apr 13, 2017

vision2910 commented Jul 14, 2016 •

edited

Loading

swapab Oct 8, 2016 •

edited

Loading