Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Various wiki bug fixes #2996

Merged
merged 2 commits into from
Nov 28, 2017
Merged

Various wiki bug fixes #2996

merged 2 commits into from
Nov 28, 2017

Conversation

ethantkoenig
Copy link
Member

@ethantkoenig ethantkoenig commented Nov 27, 2017 •
edited
Loading

This PR does the following:

techknowlogick
techknowlogick reviewed Nov 27, 2017
View reviewed changes
models/unit_tests.go Outdated
@@ -32,7 +32,6 @@ func CreateTestEngine(fixturesDir string) error {
if err = x.StoreEngine("InnoDB").Sync2(tables...); err != nil {
return err
}
x.ShowSQL(true)
Copy link
Member

Choose a reason for hiding this comment

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

This change is from your other PR #2995 and unrelated to this PR. It is unlikely to create merge conflicts, but perhaps it should be removed just in case.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

techknowlogick
techknowlogick reviewed Nov 27, 2017
View reviewed changes
models/twofactor.go
@@ -61,7 +61,7 @@ func (t *TwoFactor) getEncryptionKey() []byte {

// SetSecret sets the 2FA secret.
func (t *TwoFactor) SetSecret(secret string) error {
secretBytes, err := com.AESEncrypt(t.getEncryptionKey(), []byte(secret))
Copy link
Member

Choose a reason for hiding this comment

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

The changes to 2FA are unrelated to this PR, could they be pulled out an resubmitted under a diff PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are necessary, because of the macaron update.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification :)

@ethantkoenig ethantkoenig force-pushed the fix/wiki branch 2 times, most recently from ab49b95 to 01d5a88 Compare November 27, 2017 05:30
@lunny lunny added the type/bug label Nov 27, 2017
@lunny lunny added this to the 1.4.0 milestone Nov 27, 2017
lunny
lunny requested changes Nov 27, 2017
View reviewed changes
modules/templates/helper.go Outdated
@@ -25,6 +25,7 @@ import (
"golang.org/x/net/html/charset"
"golang.org/x/text/transform"
"gopkg.in/editorconfig/editorconfig-core-go.v1"
"net/url"
Copy link
Member

Choose a reason for hiding this comment

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

wrong import order

Copy link
Member Author

Choose a reason for hiding this comment

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

done

templates/repo/wiki/view.tmpl
@@ -85,7 +85,7 @@
{{end}}
</div>
{{if .footerPresent}}
<div class="ui grey segment">
<div class="ui segment">
Copy link
Member

Choose a reason for hiding this comment

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

why removegrey?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the footer was the only one with a grey line on the top; neither the main wiki page nor the sidebar had one, so I removed it for consistency.

@lafriks
Copy link
Member

lafriks commented Nov 27, 2017

Tests seem to fail

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 27, 2017
@codecov-io
Copy link

codecov-io commented Nov 28, 2017 •
edited
Loading

Codecov Report

Merging #2996 into master will increase coverage by 0.18%.
The diff coverage is 15.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2996      +/-   ##
==========================================
+ Coverage   32.53%   32.72%   +0.18%     
==========================================
  Files         267      267              
  Lines       39271    39188      -83     
==========================================
+ Hits        12778    12825      +47     
+ Misses      24700    24548     -152     
- Partials     1793     1815      +22
Impacted Files Coverage Δ
routers/routes/routes.go 86.34% <ø> (-0.03%) ⬇️
modules/templates/helper.go 52.47% <ø> (ø) ⬆️
routers/repo/wiki.go 3.05% <0%> (-0.02%) ⬇️
models/error.go 28.93% <0%> (-0.48%) ⬇️
models/twofactor.go 7.69% <0%> (ø) ⬆️
modules/context/context.go 48.97% <100%> (ø) ⬆️
models/wiki.go 52.51% <53.48%> (+40.21%) ⬆️
models/repo_indexer.go 49% <0%> (-2.98%) ⬇️
modules/indexer/repo.go 60.86% <0%> (-2.61%) ⬇️
... and 2 more

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 6a58e3f...ad14378. Read the comment docs.

lafriks
lafriks reviewed Nov 28, 2017
View reviewed changes
routers/routes/routes.go
@@ -608,7 +608,6 @@ func RegisterRoutes(m *macaron.Macaron) {

m.Group("/wiki", func() {
m.Get("/raw/*", repo.WikiRaw)
m.Get("/*", repo.WikiRaw)
Copy link
Member

Choose a reason for hiding this comment

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

Why this route is removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It cannot be reached (aside from a few corner cases), since the /:page route takes precedence.

@lunny
Copy link
Member

lunny commented Nov 28, 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 28, 2017
@lafriks
Copy link
Member

lafriks commented Nov 28, 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 28, 2017
@lunny lunny merged commit b7ebaf6 into go-gitea:master Nov 28, 2017
@ethantkoenig ethantkoenig deleted the fix/wiki branch November 28, 2017 18:04
@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.
Reviewers

@lafriks lafriks lafriks left review comments

@techknowlogick techknowlogick techknowlogick left review comments

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.4.0
6 participants
@ethantkoenig @lafriks @codecov-io @lunny @techknowlogick @tboerger