-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Apply LANDING_PAGE config options for logged in users #2894
Conversation
…tch for 'organizations' in addition to 'home' and 'explore'.
Signed-off-by: Mike Schaffer <mschaff@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #2894 +/- ##
==========================================
- Coverage 27.26% 27.24% -0.02%
==========================================
Files 89 89
Lines 17640 17640
==========================================
- Hits 4809 4806 -3
- Misses 12144 12146 +2
- Partials 687 688 +1
Continue to review full report at Codecov.
|
I think I see a downside of applying this to logged in users as well. If it is applied to them, and say for example the landing page is set to organizations, then no one would be able to see the news feed as the redirect would always happen. |
@techknowlogick You're right. I didn't catch that the Dashboard link just redirects to LANDING_PAGE. I'll close this PR since this needs some additional thought about how to retain the news feed. Is there a news URL to which the Dashboard button could be explicitly linked? |
One option would be to redirect after successful login to landing page |
… added switch for 'organizations' in addition to 'home' and 'explore'. Signed-off-by: Mike Schaffer <mschaff@gmail.com>
@lafriks OK. Will reopen the PR with this suggestion. One limitation is that it will only trigger when a user first logs in and not for subsequent sessions. |
modules/context/auth.go
Outdated
@@ -42,7 +42,7 @@ func Toggle(options *ToggleOptions) macaron.Handler { | |||
ctx.Redirect(setting.AppSubURL + string(setting.LandingPageURL)) | |||
return | |||
} | |||
|
|||
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.
Please remove this change
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.
Done.
routers/user/auth.go
Outdated
@@ -97,7 +97,11 @@ func checkAutoLogin(ctx *context.Context) bool { | |||
ctx.SetCookie("redirect_to", "", -1, setting.AppSubURL) | |||
ctx.Redirect(redirectTo) | |||
} else { | |||
ctx.Redirect(setting.AppSubURL + "/") | |||
if setting.LandingPageURL != setting.LandingPageHome { |
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 there is no need for this as this would do the same: ctx.Redirect(setting.AppSubURL + string(setting.LandingPageURL))
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.
Agreed.
…tch for 'organizations' in addition to 'home' and 'explore'.
Signed-off-by: Mike Schaffer <mschaff@gmail.com>
… added switch for 'organizations' in addition to 'home' and 'explore'. Signed-off-by: Mike Schaffer <mschaff@gmail.com>
Signed-off-by: Mike Schaffer <mschaff@gmail.com>
Signed-off-by: Mike Schaffer <mschaff@gmail.com>
conf/app.ini
Outdated
IS_INPUT_FILE = false |
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.
Also revert this change as it's not related to this PR
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.
This was an editor artifact but it's now reverted. Every editor seems to introduce a final EOF new line character (vi, BBEdit, Atom, GithHub's built-in editor). Finally had to use some perl to remove it: perl -pi -e 'chomp if eof' conf/app.ini. Any reason why this file doesn't end with a new line?
Also see CI failure (there is some spacing problem). Run |
Reverted new line.
Signed-off-by: Mike Schaffer <mschaff@gmail.com>
LGTM |
LGTM |
Allows LANDING_PAGE config variable options for logged in users and extends these options to include "home", "explore", and now "organizations". Addresses issue #2893.