Skip to content

Conversation

schaffman5
Copy link
Contributor

Allows LANDING_PAGE config variable options for logged in users and extends these options to include "home", "explore", and now "organizations". Addresses issue #2893.

…tch for 'organizations' in addition to 'home' and 'explore'.
Signed-off-by: Mike Schaffer <mschaff@gmail.com>
@codecov-io
Copy link

codecov-io commented Nov 12, 2017

Codecov Report

Merging #2894 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@ 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
Impacted Files Coverage Δ
routers/user/auth.go 0% <0%> (ø) ⬆️
modules/process/manager.go 69.56% <0%> (-4.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3b6383...6594fa1. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 12, 2017
@techknowlogick
Copy link
Member

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.

@schaffman5
Copy link
Contributor Author

@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?

@schaffman5 schaffman5 closed this Nov 12, 2017
@lafriks
Copy link
Member

lafriks commented Nov 12, 2017

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>
@schaffman5
Copy link
Contributor Author

@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.

@schaffman5 schaffman5 reopened this Nov 12, 2017
return
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

ctx.Redirect(redirectTo)
} else {
ctx.Redirect(setting.AppSubURL + "/")
if setting.LandingPageURL != setting.LandingPageHome {
Copy link
Member

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))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

@lafriks lafriks added the type/enhancement An improvement of existing functionality label Nov 12, 2017
@lafriks lafriks added this to the 1.4.0 milestone Nov 12, 2017
…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
RENDER_COMMAND = "asciidoc --out-file=- -"
; Input is not a standard input but a file
IS_INPUT_FILE = false
IS_INPUT_FILE = false
Copy link
Member

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

Copy link
Contributor Author

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?

@lafriks
Copy link
Member

lafriks commented Nov 12, 2017

Also see CI failure (there is some spacing problem). Run make fmt-check

Reverted new line.
Signed-off-by: Mike Schaffer <mschaff@gmail.com>
@lafriks
Copy link
Member

lafriks commented Nov 12, 2017

LGTM

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 12, 2017
@lunny
Copy link
Member

lunny commented Nov 20, 2017

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 20, 2017
@lunny lunny merged commit 7e6c198 into go-gitea:master Nov 20, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality

6 participants