Fix topics addition (Another solution) (#4031) #4258

axifive · 2018-06-16T13:31:57Z

Added topics validator (fixed the problems discussed in fix topic special tags #4031)
Fixed topics duplication in repository.topics field
Added migration file for cleanup incorrect topics

Request for check duplicates on explore or user pages:
curl 'https://try.gitea.io/USER/REPO/topics' -H 'Origin: https://try.gitea.io' -H 'Content-Type: application/x-www-form-urlencoded; charset=UTF-8' -H 'Cache-Control: no-cache' -H 'X-Requested-With: XMLHttpRequest' -H 'Cookie: lang=en-US; i_like_gitea=Your_I_LIKE_GITEA; _csrf=Your_CSRF -H 'Connection: keep-alive' -H 'DNT: 1' --data '_csrf=Your_CSRF&topics=go,test,golang,test,go'

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

daviian · 2018-06-16T19:05:51Z

@axifive Does the duplication of repository topics have any visible side affects for the user? If not I'd suggest moving that fix into a new PR and only keep the topic validator.

daviian · 2018-06-16T19:06:24Z

And please fix lint errors.

axifive · 2018-06-16T19:16:18Z

@daviian, lint errors already fixed, just drone second run missed the last commit

axifive · 2018-06-16T19:18:49Z

@daviian, ~~Ok, and I think we can delete the field repository.topics as unused, only which can have duplicates~~

daviian · 2018-06-16T19:49:27Z

@axifive I've already restarted drone after that. There are new lint errors.

axifive · 2018-06-16T20:42:21Z

@daviian, oh, I see, thank you.

techknowlogick · 2018-06-17T01:51:42Z

routers/repo/topic.go

@@ -13,7 +13,7 @@ import (
 )

 // TopicPost response for creating repository


The lint error is right here TopicPost should be TopicsPost to match name of function

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

codecov-io · 2018-06-17T12:22:31Z

Codecov Report

Merging #4258 into master will decrease coverage by <.01%.
The diff coverage is 13.51%.

@@            Coverage Diff            @@
##           master   #4258      +/-   ##
=========================================
- Coverage   20.01%     20%   -0.01%     
=========================================
  Files         153     153              
  Lines       30584   30620      +36     
=========================================
+ Hits         6122    6127       +5     
- Misses      23535   23565      +30     
- Partials      927     928       +1

Impacted Files	Coverage Δ
routers/repo/topic.go	`0% <0%> (ø)`	⬆️
models/topic.go	`54.7% <62.5%> (+0.57%)`	⬆️

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 9ae7664...0c97476. Read the comment docs.

axifive · 2018-06-17T17:30:26Z

@daviian, I found where used repository.topics. It used for repo_list template, unfortunately, GoLand сan't search by templates.
So we need cleaning requested duplicate topics or select validated topics from DB and then update repo field.

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

techknowlogick · 2018-06-18T01:25:35Z

@axifive just restarted the build and it picked up your lint changes, so it should report successful in a couple of minutes.

techknowlogick · 2018-06-18T01:26:28Z

models/migrations/v67.go

@@ -0,0 +1,137 @@
+package migrations


Can you add copyright to top of this file?

yes, missed it

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

lunny · 2018-06-18T07:39:00Z

models/migrations/v67.go

+	log.Info("Validating existed topics...")
+	for start := 0; ; start += batchSize {
+		topics = topics[:0]
+		if err := x.Asc("id").Limit(batchSize, start).Find(&topics); err != nil {


If you deleted some topics, this will ignore some topics.

Fixed it, Thanks!

I don't think it has been fixed. I think maybe you can save deleting topics to a slice and deleted them after the traversing.

@lunny Why do you think so? How can I check this wrong behavior?

@axifive because you have deleted the topics when your traversing the topics.

@lunny, Ok, I'll rewrite this.

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

lafriks · 2018-06-19T09:01:10Z

models/topic.go

@@ -20,6 +21,8 @@ func init() {
 	)
 }

+var topicPattern = regexp.MustCompile(`^[a-z0-9+#_.-]+$`)


I think we should limit same as github does with regexp: ^[a-z0-9][a-z0-9-]*$

Ok, Changed it

lafriks · 2018-06-19T09:04:06Z

options/locale/locale_en-US.ini

@@ -1167,6 +1167,8 @@ branch.protected_deletion_failed = Branch '%s' is protected. It cannot be delete

 topic.manage_topics = Manage Topics
 topic.done = Done
+topic.count_prompt = You can't select more than 25 topics
+topic.format_prompt = Topics must use letter or number and can include hyphen(-), underscore(_), plus(+), hash(#), dot(.) with max length of 35


And error message if changed to the regexp above would be: Topics must start with letter or number, can include hyphen(-) must be no more than 35 characters long or something like that :)

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

daviian · 2018-06-19T13:48:45Z

models/migrations/v67.go

+	}
+
+	log.Info("Deleting incorrect topics...")
+	for start := 0; ; start+=batchSize {


already. Need restart drone task, it got: "RPC failed" error

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

jonasfranz · 2018-06-19T14:57:20Z

Topics should be already checked in JS.

Example: If I type <iframe> and press enter, an iframe gets rendered. A validation error should be shown instead.

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

axifive · 2018-06-19T15:01:34Z

@JonasFranzDEV I'll add this by another PR, I already have non-complete solution

jonasfranz · 2018-06-19T15:03:37Z

routers/repo/topic.go

+	topics = topics[:i]
+
+	if len(topics) > 25 {
+		log.Error(2, "Incorrect number of topics(max 25)")


I don't think that this must be logged because the user receives the error message already. If you nevertheless want to log it, please use log.Info or log.Warn instead, since this is not a "real" error.

That's reasonable, thank you

jonasfranz · 2018-06-19T15:03:51Z

routers/repo/topic.go

+	}
+
+	if len(invalidTopics) > 0 {
+		log.Error(2, "Invalid topics: %v", invalidTopics)


Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

jonasfranz · 2018-06-20T08:14:34Z

models/topic.go

@@ -51,6 +54,11 @@ func (err ErrTopicNotExist) Error() string {
 	return fmt.Sprintf("topic is not exist [name: %s]", err.Name)
 }

+// TopicValidator checks topics by length and match pattern rules
+func TopicValidator(topic string) bool {


I don‘t like the name of this function because it sounds like a struct/interface, Please rename it to ValidateTopic.

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

lafriks · 2018-06-20T10:13:20Z

@JonasFranzDEV @daviian @lunny please approve

jonasfranz · 2018-06-20T11:58:43Z

Needs @daviian 's and @lunny 's approval

axifive added 4 commits June 16, 2018 05:57

Added topics validation, fixed repo topics duplication (go-gitea#4031)

42096c6

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

Added tests

f2546a7

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

Fixed fmt

b0e7d00

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

Added comments to exported functions

6579ddf

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 16, 2018

lunny added this to the 1.5.0 milestone Jun 17, 2018

lunny added the type/bug label Jun 17, 2018

lunny mentioned this pull request Jun 17, 2018

fix topic special tags #4031

Closed

techknowlogick reviewed Jun 17, 2018

View reviewed changes

axifive added 2 commits June 17, 2018 14:48

Deleted RemoveDuplicateTopics function

630ca59

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

Fixed messages

c1364ea

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

axifive added 3 commits June 18, 2018 03:18

Added migration

7f1ec18

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

fmt migration file

1811a6b

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

fixed lint

cab094e

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

techknowlogick reviewed Jun 18, 2018

View reviewed changes

axifive added 2 commits June 18, 2018 04:44

Added Copyright

8fee5d3

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

Added query solution for duplicates

9141482

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

lunny requested changes Jun 18, 2018

View reviewed changes

Fixed migration query

613f2b5

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

lafriks requested changes Jun 19, 2018

View reviewed changes

Changed RegExp. Fixed migration

c1da9ae

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

daviian suggested changes Jun 19, 2018

View reviewed changes

fmt migration file

e326026

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

Fixed test for changed regexp

4fb3b1a

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

jonasfranz suggested changes Jun 19, 2018

View reviewed changes

axifive added 3 commits June 19, 2018 18:29

Removed validation log messages

8c51851

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

Renamed migration file

ffc96b0

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

Merge branch 'master' into fix-topics-addition

21ff04a

lafriks approved these changes Jun 20, 2018

View reviewed changes

bkcsoft 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 Jun 20, 2018

Merge branch 'master' into fix-topics-addition

3c4517f

jonasfranz suggested changes Jun 20, 2018

View reviewed changes

Renamed validate function

b166e69

Signed-off-by: Alexey Terentyev <axifnx@gmail.com>

lafriks approved these changes Jun 20, 2018

View reviewed changes

jonasfranz approved these changes Jun 20, 2018

View reviewed changes

bkcsoft 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 Jun 20, 2018

axifive mentioned this pull request Jun 20, 2018

hide issues from org private repos w/o team assignment #4034

Merged

lunny approved these changes Jun 21, 2018

View reviewed changes

Merge branch 'master' into fix-topics-addition

61aaa2a

daviian approved these changes Jun 21, 2018

View reviewed changes

Merge branch 'master' into fix-topics-addition

0c97476

lunny merged commit 46d19c4 into go-gitea:master Jun 21, 2018

axifive deleted the fix-topics-addition branch June 21, 2018 16:04

Treora mentioned this pull request Jun 23, 2018

Migration for topic validation fails #4303

Closed

5 tasks

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix topics addition (Another solution) (#4031) #4258

Fix topics addition (Another solution) (#4031) #4258

axifive commented Jun 16, 2018 •

edited

Loading

daviian commented Jun 16, 2018

daviian commented Jun 16, 2018

axifive commented Jun 16, 2018

axifive commented Jun 16, 2018 •

edited

Loading

daviian commented Jun 16, 2018

axifive commented Jun 16, 2018

techknowlogick Jun 17, 2018

axifive Jun 17, 2018

codecov-io commented Jun 17, 2018 •

edited

Loading

axifive commented Jun 17, 2018 •

edited

Loading

techknowlogick commented Jun 18, 2018

techknowlogick Jun 18, 2018

axifive Jun 18, 2018

lunny Jun 18, 2018

axifive Jun 18, 2018

lunny Jun 18, 2018

axifive Jun 18, 2018

lunny Jun 18, 2018

axifive Jun 18, 2018

lafriks Jun 19, 2018

axifive Jun 19, 2018

lafriks Jun 19, 2018

axifive Jun 19, 2018

daviian Jun 19, 2018

axifive Jun 19, 2018 •

edited

Loading

jonasfranz commented Jun 19, 2018 •

edited

Loading

axifive commented Jun 19, 2018 •

edited

Loading

jonasfranz Jun 19, 2018

axifive Jun 19, 2018

jonasfranz Jun 19, 2018

jonasfranz Jun 20, 2018

lafriks commented Jun 20, 2018

jonasfranz commented Jun 20, 2018

		@@ -13,7 +13,7 @@ import (
		)

		// TopicPost response for creating repository

Fix topics addition (Another solution) (#4031) #4258

Fix topics addition (Another solution) (#4031) #4258

Conversation

axifive commented Jun 16, 2018 • edited Loading

daviian commented Jun 16, 2018

daviian commented Jun 16, 2018

axifive commented Jun 16, 2018

axifive commented Jun 16, 2018 • edited Loading

daviian commented Jun 16, 2018

axifive commented Jun 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 17, 2018 • edited Loading

Codecov Report

axifive commented Jun 17, 2018 • edited Loading

techknowlogick commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axifive Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

jonasfranz commented Jun 19, 2018 • edited Loading

axifive commented Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Jun 20, 2018

jonasfranz commented Jun 20, 2018

axifive commented Jun 16, 2018 •

edited

Loading

axifive commented Jun 16, 2018 •

edited

Loading

codecov-io commented Jun 17, 2018 •

edited

Loading

axifive commented Jun 17, 2018 •

edited

Loading

axifive Jun 19, 2018 •

edited

Loading

jonasfranz commented Jun 19, 2018 •

edited

Loading

axifive commented Jun 19, 2018 •

edited

Loading