Specify a default admin context in site settings #443

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
@jfox015
Contributor

jfox015 commented May 23, 2012

I added a settings option in site settings to select a default context so it can be changed by the site admin without requiring a code change. The list function in settings only lists contexts with landing (index.php) files to avoid infinite loops.

@seandowney

This comment has been minimized.

Show comment Hide comment
@seandowney

seandowney May 29, 2012

Owner

@jfox015 do you think this is needed especially with the fact that you can set the default context on a role basis?

//cc @lonnieezell //cc @svizion What do ye think?

Owner

seandowney commented May 29, 2012

@jfox015 do you think this is needed especially with the fact that you can set the default context on a role basis?

//cc @lonnieezell //cc @svizion What do ye think?

@lonnieezell

This comment has been minimized.

Show comment Hide comment
@lonnieezell

lonnieezell May 30, 2012

Owner

This sounds like a duplicate of the login destination for each role. @jfox015 What use-case do you see for this that login roles doesn't provide?

Owner

lonnieezell commented May 30, 2012

This sounds like a duplicate of the login destination for each role. @jfox015 What use-case do you see for this that login roles doesn't provide?

@svizion

This comment has been minimized.

Show comment Hide comment
@svizion

svizion May 30, 2012

Contributor

I couldn't really think much of a use case myself since there is a home controller in the admin that redirects you to somewhere and the roles have a redirect also.

Contributor

svizion commented May 30, 2012

I couldn't really think much of a use case myself since there is a home controller in the admin that redirects you to somewhere and the roles have a redirect also.

@jfox015

This comment has been minimized.

Show comment Hide comment
@jfox015

jfox015 May 30, 2012

Contributor

@lonnieezell @svizion @seandowney
I must have missed the roles redirect. Where is that at? I'll look and if I see a duplication of functionality.

My personal use case was a site admin who wanted to set a specific default context for their application for all other dashboard users. But if each rol,e can be assigned a specific context, that would negate the need for this.

Contributor

jfox015 commented May 30, 2012

@lonnieezell @svizion @seandowney
I must have missed the roles redirect. Where is that at? I'll look and if I see a duplication of functionality.

My personal use case was a site admin who wanted to set a specific default context for their application for all other dashboard users. But if each rol,e can be assigned a specific context, that would negate the need for this.

@seandowney

This comment has been minimized.

Show comment Hide comment
@seandowney

seandowney May 30, 2012

Owner

@jfox015 You can edit the Login Destination for a role when editing the role itself in the Roles module.

Owner

seandowney commented May 30, 2012

@jfox015 You can edit the Login Destination for a role when editing the role itself in the Roles module.

@jfox015

This comment has been minimized.

Show comment Hide comment
@jfox015

jfox015 May 31, 2012

Contributor

@seandowney
I see the ability to specify the landing page, but when I enter my custom context, it doesn't use it. I don;t see where this would work seeing how the value is hard coded into home.php right now.

Should I maybe move my context drop down box to this page instead of the site settings screen and change the code in /controllers/home.php to pick up this value instead?

Contributor

jfox015 commented May 31, 2012

@seandowney
I see the ability to specify the landing page, but when I enter my custom context, it doesn't use it. I don;t see where this would work seeing how the value is hard coded into home.php right now.

Should I maybe move my context drop down box to this page instead of the site settings screen and change the code in /controllers/home.php to pick up this value instead?

@svizion

This comment has been minimized.

Show comment Hide comment
@svizion

svizion May 31, 2012

Contributor

Maybe we should just add the correct redirect that's used by the role in the admin/home.php?
//cc @jfox015 @seandowney @lonnieezell

Contributor

svizion commented May 31, 2012

Maybe we should just add the correct redirect that's used by the role in the admin/home.php?
//cc @jfox015 @seandowney @lonnieezell

@jfox015

This comment has been minimized.

Show comment Hide comment
@jfox015

jfox015 May 31, 2012

Contributor

@svizion I suggested my drop down because I go through and test for landing pages for each context so that if an index.php doesn't exist (as there is not one for settings or developer), they won't be in the list. When you try to hit a context with no view it throws your into an infinite loop. A simpler fix would be to just put blank index.php page in that directory I guess, but I was thinking about an automatic safe guard for developers adding their own custom contexts as well.

Contributor

jfox015 commented May 31, 2012

@svizion I suggested my drop down because I go through and test for landing pages for each context so that if an index.php doesn't exist (as there is not one for settings or developer), they won't be in the list. When you try to hit a context with no view it throws your into an infinite loop. A simpler fix would be to just put blank index.php page in that directory I guess, but I was thinking about an automatic safe guard for developers adding their own custom contexts as well.

@seandowney

This comment has been minimized.

Show comment Hide comment
@seandowney

seandowney May 31, 2012

Owner

The role destination is supposed to take a relative path but if it's not working we should fix that.

Owner

seandowney commented May 31, 2012

The role destination is supposed to take a relative path but if it's not working we should fix that.

@seandowney

This comment has been minimized.

Show comment Hide comment
@seandowney

seandowney May 31, 2012

Owner

@jfox015 I tested the role login destination now and it works for me. I tested it by going to the "/admin/settings" page

You can specify any page you want. You mentioned that you have a custom context, then that would need a controller in /controllers/admin/ if you want to go to the base page for that context.

Is that what the requirement is?

Owner

seandowney commented May 31, 2012

@jfox015 I tested the role login destination now and it works for me. I tested it by going to the "/admin/settings" page

You can specify any page you want. You mentioned that you have a custom context, then that would need a controller in /controllers/admin/ if you want to go to the base page for that context.

Is that what the requirement is?

@jfox015

This comment has been minimized.

Show comment Hide comment
@jfox015

jfox015 May 31, 2012

Contributor

@seandowney The point of this update was that for my custom fork for the OOWP, I want to direct users entering the admin dashboard to view my context landing page first instead of the hard coded content target as it will have identification details and instructions for the site admins/editors/moderators. I did want to give site admins a choice in that or another context to show by default when users click to enter the Admin Area, hence this update.

This is separate from a login destination in that this landing page that would get hit specifically when the controllers/home.php (Admin home controller) index function is hit. So while it is similar to login destination, it actually affects the user taking a different action in the site.

Contributor

jfox015 commented May 31, 2012

@seandowney The point of this update was that for my custom fork for the OOWP, I want to direct users entering the admin dashboard to view my context landing page first instead of the hard coded content target as it will have identification details and instructions for the site admins/editors/moderators. I did want to give site admins a choice in that or another context to show by default when users click to enter the Admin Area, hence this update.

This is separate from a login destination in that this landing page that would get hit specifically when the controllers/home.php (Admin home controller) index function is hit. So while it is similar to login destination, it actually affects the user taking a different action in the site.

@seandowney

This comment has been minimized.

Show comment Hide comment
@seandowney

seandowney May 31, 2012

Owner

@jfox015 Ah right - I understand now. @svizion may have suggested it but what about using the login destination instead of the hard coded redirect in admin/home.php ? Would that cover it?

Owner

seandowney commented May 31, 2012

@jfox015 Ah right - I understand now. @svizion may have suggested it but what about using the login destination instead of the hard coded redirect in admin/home.php ? Would that cover it?

@jfox015

This comment has been minimized.

Show comment Hide comment
@jfox015

jfox015 May 31, 2012

Contributor

Ok going to throw out the question then, what if the roles destination is not in the dashboard? If someone were to go to bonfiresite.com/admin/ they'd be continuously kicked back out unless they had a direct context link to avoid that redirect.

Contributor

jfox015 commented May 31, 2012

Ok going to throw out the question then, what if the roles destination is not in the dashboard? If someone were to go to bonfiresite.com/admin/ they'd be continuously kicked back out unless they had a direct context link to avoid that redirect.

@seandowney

This comment has been minimized.

Show comment Hide comment
@seandowney

seandowney May 31, 2012

Owner

@jfox015 Ok now I really understand what you mean :-) I was typing a different message a minute ago and then realised what you meant.

So yes - if a role had a login destination outside of the admin area but they should also have access to the admin area, if it was done the way I suggested then they would never be able to get in to the admin because they would be redirected to the login destination. (Is that a correct summary?)

Now I see why we need this code - they are two separate cases.

Yes I think your suggestion here #443 (comment) is a good one - to move the context menu into the role - so each role can have a login destination and a default context. So the role record would need a new field instead of just a new site setting.

//cc @lonnieezell //cc @svizion What do ye think of this?

Owner

seandowney commented May 31, 2012

@jfox015 Ok now I really understand what you mean :-) I was typing a different message a minute ago and then realised what you meant.

So yes - if a role had a login destination outside of the admin area but they should also have access to the admin area, if it was done the way I suggested then they would never be able to get in to the admin because they would be redirected to the login destination. (Is that a correct summary?)

Now I see why we need this code - they are two separate cases.

Yes I think your suggestion here #443 (comment) is a good one - to move the context menu into the role - so each role can have a login destination and a default context. So the role record would need a new field instead of just a new site setting.

//cc @lonnieezell //cc @svizion What do ye think of this?

@jfox015

This comment has been minimized.

Show comment Hide comment
@jfox015

jfox015 Jun 1, 2012

Contributor

@seandowney I'm realizing I should have been a little clearer in my use case up front. It does sound like a more broad case like Shawn and Lonnie assumed in my explanation. I'll try to do a better job on future pull requests.

Contributor

jfox015 commented Jun 1, 2012

@seandowney I'm realizing I should have been a little clearer in my use case up front. It does sound like a more broad case like Shawn and Lonnie assumed in my explanation. I'll try to do a better job on future pull requests.

@svizion

This comment has been minimized.

Show comment Hide comment
@svizion

svizion Jun 4, 2012

Contributor

I'm pretty good with this idea honestly, I haven't really needed this kind of functionality before but can't see any harm in adding it.

Contributor

svizion commented Jun 4, 2012

I'm pretty good with this idea honestly, I haven't really needed this kind of functionality before but can't see any harm in adding it.

@seandowney

This comment has been minimized.

Show comment Hide comment
@seandowney

seandowney Jun 5, 2012

Owner

Ok cool - @jfox015 would you be able to move this into the Role?

//cc @lonnieezell Any objections?

Owner

seandowney commented Jun 5, 2012

Ok cool - @jfox015 would you be able to move this into the Role?

//cc @lonnieezell Any objections?

@lonnieezell

This comment has been minimized.

Show comment Hide comment
@lonnieezell

lonnieezell Jun 5, 2012

Owner

Sounds good to me.

Owner

lonnieezell commented Jun 5, 2012

Sounds good to me.

@jfox015

This comment has been minimized.

Show comment Hide comment
@jfox015

jfox015 Jun 6, 2012

Contributor

I can move it to the role settings. This would still be "default admin context" separate from the login redirect, correct?

Contributor

jfox015 commented Jun 6, 2012

I can move it to the role settings. This would still be "default admin context" separate from the login redirect, correct?

@seandowney

This comment has been minimized.

Show comment Hide comment
@seandowney

seandowney Jun 6, 2012

Owner

Yes, I think we will need some explanation text beside both :-)

Owner

seandowney commented Jun 6, 2012

Yes, I think we will need some explanation text beside both :-)

@jfox015

This comment has been minimized.

Show comment Hide comment
@jfox015

jfox015 Jun 6, 2012

Contributor

@seandowney OK, I'll note what each does in help captions. Will commit this tonight.

Contributor

jfox015 commented Jun 6, 2012

@seandowney OK, I'll note what each does in help captions. Will commit this tonight.

@seandowney

This comment has been minimized.

Show comment Hide comment
@seandowney

seandowney Jun 6, 2012

Owner

@jfox015 cool thanks sounds great

Owner

seandowney commented Jun 6, 2012

@jfox015 cool thanks sounds great

+ {
+ $this->load->model('role_model');
+ }
+ $user_role = $this->role_model->find($this->current_user->role_id);

This comment has been minimized.

Show comment Hide comment
@jfox015

jfox015 Jun 7, 2012

Contributor

@seandowney @svizion
I have this 99.5% done, but for some reason I am getting a

Fatal error: Call to a member function find() on a non-object in I:\My Web Sites\ootp_web\html\ootp_online\bonfire\application\controllers\admin\home.php on line 59

error on this line. Any thoughts?

@jfox015

jfox015 Jun 7, 2012

Contributor

@seandowney @svizion
I have this 99.5% done, but for some reason I am getting a

Fatal error: Call to a member function find() on a non-object in I:\My Web Sites\ootp_web\html\ootp_online\bonfire\application\controllers\admin\home.php on line 59

error on this line. Any thoughts?

This comment has been minimized.

Show comment Hide comment
@svizion

svizion Jun 7, 2012

Contributor

@jfox015

I haven't had a chance to review the code yet, but try taking your check point out when loading the role_model and make sure the role_model is extending BF_Model those are just some thoughts of top of my head, I'll try and take a look at the code itself and see if I can find any issues

Maybe instead of using $this->role_model try using

    if (!class_exists('Role_model'))
    {
      $this->load->model('roles/role_model');
    }

Not sure if that will help or not but I normally check if a class is loaded that way instead of using isset

@svizion

svizion Jun 7, 2012

Contributor

@jfox015

I haven't had a chance to review the code yet, but try taking your check point out when loading the role_model and make sure the role_model is extending BF_Model those are just some thoughts of top of my head, I'll try and take a look at the code itself and see if I can find any issues

Maybe instead of using $this->role_model try using

    if (!class_exists('Role_model'))
    {
      $this->load->model('roles/role_model');
    }

Not sure if that will help or not but I normally check if a class is loaded that way instead of using isset

This comment has been minimized.

Show comment Hide comment
@seandowney

seandowney Jun 7, 2012

Owner

Or try checking if the role_model class exists rather than using "isset"

@seandowney

seandowney Jun 7, 2012

Owner

Or try checking if the role_model class exists rather than using "isset"

This comment has been minimized.

Show comment Hide comment
@jfox015

jfox015 Jun 7, 2012

Contributor

@seandowney @svizion thanks guys. I'll try that later and see if that fixes the issue.

@jfox015

jfox015 Jun 7, 2012

Contributor

@seandowney @svizion thanks guys. I'll try that later and see if that fixes the issue.

@jfox015

This comment has been minimized.

Show comment Hide comment
@jfox015

jfox015 Jun 8, 2012

Owner

This turned out to be the culprit. The role object was coming back false. Works now.

Owner

jfox015 commented on b899e9f Jun 8, 2012

This turned out to be the culprit. The role object was coming back false. Works now.

@seandowney

This comment has been minimized.

Show comment Hide comment
@seandowney

seandowney Jun 20, 2012

Owner

@jfox015 Sorry this is a bit out of sync with the current develop

Owner

seandowney commented Jun 20, 2012

@jfox015 Sorry this is a bit out of sync with the current develop

@jfox015

This comment has been minimized.

Show comment Hide comment
@jfox015

jfox015 Jun 21, 2012

Contributor

@seandowney I think this branch has been corrupted a bit as some files that shouldn't have changes are showing up. I'm going to close this pull request, create a new branch and resubmit again.

Contributor

jfox015 commented Jun 21, 2012

@seandowney I think this branch has been corrupted a bit as some files that shouldn't have changes are showing up. I'm going to close this pull request, create a new branch and resubmit again.

@jfox015 jfox015 closed this Jun 21, 2012

@seandowney

This comment has been minimized.

Show comment Hide comment
@seandowney

seandowney Jun 21, 2012

Owner

Ok thanks

Owner

seandowney commented Jun 21, 2012

Ok thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment