Improve CBF topics #36

Gusted · 2022-07-16T13:46:44+02:00

Gusted commented

2022-07-16 13:46:44 +02:00

Topics that are prefix with cbf- will now receive special care within the code. It can only be removed and viewed by Instance admins. Repository owners are no longer able to remove these topics or even see that this topic was added to their repository(just the warning banner).
Because users don't receive the cbf- prefixed topics we need to give some special care when users do update their topics, this PR adds the AddMissingRepositoryTopics function which will make sure that all current cbf- prefixed topics will be added to the updated topics list.
The template to show the error will now call the HasMissingLicenseTopic function as the Topics slice no longer contains the cbf- prefixed topics.
Users are no longer able to search keywords that includes the cbf- text, this to avoid accidental leakage of repository that has cbf- topics.

- Topics that are prefix with `cbf-` will now receive special care within the code. It can only be removed and viewed by Instance admins. Repository owners are no longer able to remove these topics or even see that this topic was added to their repository(just the warning banner). - Because users don't receive the `cbf-` prefixed topics we need to give some special care when users do update their topics, this PR adds the `AddMissingRepositoryTopics` function which will make sure that all current `cbf-` prefixed topics will be added to the updated topics list. - The template to show the error will now call the `HasMissingLicenseTopic` function as the Topics slice no longer contains the `cbf-` prefixed topics. - Users are no longer able to search keywords that includes the `cbf-` text, this to avoid accidental leakage of repository that has `cbf-` topics.

👍 1 ❤️ 1

Gusted added 1 commit

2022-07-16 13:47:06 +02:00

Improve CBF topics 49bb717d4b

- Topics that are prefix with `cbf-` will now receive special care
within the code. It can only be removed and viewed by Instance admins.
Repository owners are no longer able to remove these topics or even see
that this topic was added to their repository(just the warning banner).
- Because users don't receive the `cbf-` prefixed topics we need to give
some special care when users do update their topics, this PR adds the
`AddMissingRepositoryTopics` function which will make sure that all
current `cbf-` prefixed topics will be added to the updated topics list.
- The template to show the error will now call the
`HasMissingLicenseTopic` function as the Topics slice no longer contains
the `cbf-` prefixed topics.
- Users are no longer able to search keywords that includes the `cbf-`
text, this to avoid accidental leakage of repository that has `cbf-`
topics.

Gusted added 1 commit

2022-07-21 01:18:25 +02:00

Merge branch 'codeberg-1.17' into improve-cbf-topics 7c80ee313b

6543 added 1 commit

2022-07-24 22:44:32 +02:00

Merge branch 'codeberg-1.17' into improve-cbf-topics 7c24ad87b6

6543 commented

2022-07-24 23:10:09 +02:00

as normal repoAdmin (not instance admin) you can add cbf-... topics but not remove or see them ... (via webUI)

as normal repoAdmin (not instance admin) you can add `cbf-...` topics but not remove or see them ... (via webUI)

6543 reviewed

2022-07-24 23:10:58 +02:00

models/repo/codeberg.go

					
				@ -18,0 +39,4 @@

				// HasMissingLicenseTopic returns if repository has a "cbf-nolicense" or "cbf-abuse-nolicense" topic.

				func (r *Repository) HasMissingLicenseTopic() bool {

					for _, topic := range r.Topics {

						if topic == "cbf-nolicense" || topic == "cbf-abuse-nolicense" {

can we create a const block where we put these strings to?

Why? The function is specific to that flag, I don't see a need to define this somewhere. For other labels, you obviously define new functions anyway.

Why? The function is specific to *that* flag, I don't see a need to define this somewhere. For other labels, you obviously define new functions anyway.

I just somehow think that these functions are obviously less performant than scanning in the template, aren't they? I mean, previously I looped through the topics twice, once to print them and once to display banners.

Now we iterate through them for each check. e.g. in the future, if the templates do check
HasMissingLicenseTopic(), HasStorageAbuseTopic(), HasStorageExceptionTopic, HasEventXyParticipationTopic(), HasBonusFeatureAbcTopic() ... we iterate through the topics many times, right?

I just somehow think that these functions are obviously less performant than scanning in the template, aren't they? I mean, previously I looped through the topics twice, once to print them and once to display banners. Now we iterate through them for each check. e.g. in the future, if the templates do check HasMissingLicenseTopic(), HasStorageAbuseTopic(), HasStorageExceptionTopic, HasEventXyParticipationTopic(), HasBonusFeatureAbcTopic() ... we iterate through the topics many times, right?

diff --git a/models/repo/codeberg.go b/models/repo/codeberg.go
index a7f1b7eb4..c212d8322 100644
--- a/models/repo/codeberg.go
+++ b/models/repo/codeberg.go
@@ -13,6 +13,12 @@ import (
        user_model "code.gitea.io/gitea/models/user"
 )
 
+const (
+       codebergTopicTrefix  = "cbf-"
+       missingLicenseTopic1 = "cbf-nolicense"
+       missingLicenseTopic2 = "cbf-abuse-nolicense"
+)
+
 // check if a repo topic is used as a Codeberg flag
 func (t *Topic) IsCodebergFlag() bool {
        return strings.HasPrefix(t.Name, "cbf-")
@@ -23,7 +29,7 @@ func (t *Topic) IsCodebergFlag() bool {
 func (r *Repository) AddMissingRepositoryTopics(topics []string) []string {
 outercheck:
        for _, topic := range r.Topics {
-               if strings.HasPrefix(topic, "cbf-") {
+               if strings.HasPrefix(topic, codebergTopicTrefix) {
                        // Avoid duplicates.
                        for _, vTopic := range topics {
                                if vTopic == topic {
@@ -39,7 +45,7 @@ outercheck:
 // HasMissingLicenseTopic returns if repository has a "cbf-nolicense" or "cbf-abuse-nolicense" topic.
 func (r *Repository) HasMissingLicenseTopic() bool {
        for _, topic := range r.Topics {
-               if topic == "cbf-nolicense" || topic == "cbf-abuse-nolicense" {
+               if topic == missingLicenseTopic1 || topic == missingLicenseTopic2 {
                        return true
                }
        }

it improves codestile ...

```diff diff --git a/models/repo/codeberg.go b/models/repo/codeberg.go index a7f1b7eb4..c212d8322 100644 --- a/models/repo/codeberg.go +++ b/models/repo/codeberg.go @@ -13,6 +13,12 @@ import ( user_model "code.gitea.io/gitea/models/user" ) +const ( + codebergTopicTrefix = "cbf-" + missingLicenseTopic1 = "cbf-nolicense" + missingLicenseTopic2 = "cbf-abuse-nolicense" +) + // check if a repo topic is used as a Codeberg flag func (t *Topic) IsCodebergFlag() bool { return strings.HasPrefix(t.Name, "cbf-") @@ -23,7 +29,7 @@ func (t *Topic) IsCodebergFlag() bool { func (r *Repository) AddMissingRepositoryTopics(topics []string) []string { outercheck: for _, topic := range r.Topics { - if strings.HasPrefix(topic, "cbf-") { + if strings.HasPrefix(topic, codebergTopicTrefix) { // Avoid duplicates. for _, vTopic := range topics { if vTopic == topic { @@ -39,7 +45,7 @@ outercheck: // HasMissingLicenseTopic returns if repository has a "cbf-nolicense" or "cbf-abuse-nolicense" topic. func (r *Repository) HasMissingLicenseTopic() bool { for _, topic := range r.Topics { - if topic == "cbf-nolicense" || topic == "cbf-abuse-nolicense" { + if topic == missingLicenseTopic1 || topic == missingLicenseTopic2 { return true } } ``` it improves codestile ...

😕 1

I'm fine with making cbf- a const. But the specific licenses is either a global slice or just hardcoded within the function.

I'm fine with making `cbf-` a const. But the specific licenses is either a global slice or just hardcoded within the function.

6543 marked this conversation as resolved

Gusted commented

2022-07-24 23:25:10 +02:00

as normal repoAdmin (not instance admin) you can add cbf-... topics but not remove or see them ... (via webUI)

Yeah, if users want to break their repository. They should free to do so! (Forget to check for this endpoint).

> as normal repoAdmin (not instance admin) you can add cbf-... topics but not remove or see them ... (via webUI) Yeah, if users want to break their repository. They should free to do so! (Forget to check for this endpoint).

6543 added 1 commit

2022-07-24 23:27:46 +02:00

Merge branch 'codeberg-1.17' into improve-cbf-topics 19cc8494f5

6543 commented

2022-07-24 23:29:53 +02:00

from @fnetX

"... I just somehow think that these functions are obviously less performant than scanning in the template, aren't they? I mean, previously I looped through the topics twice, once to print them and once to display banners."

from @fnetX > "... I just somehow think that these functions are obviously less performant than scanning in the template, aren't they? I mean, previously I looped through the topics twice, once to print them and once to display banners."

6543 commented

2022-07-24 23:31:23 +02:00

I would say it depends how mouch overhead the template engine does add to in-template-functions.

Gusted commented

2022-07-24 23:53:25 +02:00

Templates have huge overhead when doing computations(especially for-loops). It's not a general purposed dynamic scripting language. It's to construct a HTML page.

If you would like to run your benchmarks: https://codeberg.org/Gusted/benchmark-loops-go-template

-> % go test -bench .           
goos: linux
goarch: amd64
pkg: gusted-bench
cpu: AMD Ryzen 5 3600X 6-Core Processor             
BenchmarkLoopNative-12            740875              2700 ns/op
BenchmarkLoopTempl-12             104628             10189 ns/op
PASS
ok      gusted-bench    3.203s

Templates have huge overhead when doing computations(especially for-loops). It's not a general purposed dynamic scripting language. It's to construct a HTML page. If you would like to run your benchmarks: https://codeberg.org/Gusted/benchmark-loops-go-template ``` -> % go test -bench . goos: linux goarch: amd64 pkg: gusted-bench cpu: AMD Ryzen 5 3600X 6-Core Processor BenchmarkLoopNative-12 740875 2700 ns/op BenchmarkLoopTempl-12 104628 10189 ns/op PASS ok gusted-bench 3.203s ```

👍 1 ❤️ 1

6543 commented

2022-07-25 00:05:16 +02:00

we can run a lot of loops before we have to worry about golang overhead 😆

6543 reviewed

2022-07-25 00:07:10 +02:00

templates/repo/codeberg_banners.tmpl Outdated

					
				@ -44,2 +39,3 @@

						{{end}}

					</div>

				{{end}}

why rm last \n ?

Not sure WTF Gitea is doing here(try to click on expanding the code below this hunk). But there is another new line there(this just removes the other new line). So 2 empty last new lines -> 1 empty last new line.

6543 marked this conversation as resolved

6543 approved these changes

2022-07-25 00:08:05 +02:00

6543 left a comment

beside file format nit

6543 approved these changes

2022-07-25 00:31:40 +02:00

6543 added 1 commit

2022-07-25 00:31:59 +02:00

rm not needed space 58a0298bc0

6543 commented

2022-07-25 00:32:08 +02:00

@Gusted 58a0298bc0

@Gusted 58a0298bc0c16aca62757e4ce58b5f3157e4885b

👍 1

Gusted commented

2022-07-25 00:40:12 +02:00

Please check if the rendered HTML still looks fine. I just copied pasted it.

6543 commented

2022-07-25 03:45:52 +02:00

![](https://codeberg.org/attachments/4ae84a7a-38ad-4ef6-a861-ac005af292ce)

2022-07-25_03-45.png

25 KiB

Gusted commented

2022-07-25 06:56:47 +02:00

Weird, it current isn't horizontal centered:

Ref: https://codeberg.org/nogerine/solar

Weird, it current isn't horizontal centered: ![image](/attachments/6e002fc6-e194-424a-b0af-efc4ff5b9c6c) Ref: https://codeberg.org/nogerine/solar

image.png

25 KiB

fnetX commented

2022-07-25 09:47:48 +02:00

Yeah, horizontal centering is hard to read with multiline text, so I made it align left. But maybe I forgot to add that to some of the banners.

6543 commented

2022-07-25 14:43:45 +02:00

I think we can ad the fix in here

Gusted commented

2022-07-27 00:30:21 +02:00

I'm not sure which banners are incorrect. So @6543 feel free to mess with my PR to add a fix.

6543 self-assigned this

2022-07-27 12:22:21 +02:00

6543 referenced this pull request from Codeberg/Community

2022-08-01 00:55:18 +02:00

Add license to existing repo / License reminder #293

fnetX commented

2022-10-28 16:27:04 +02:00

My proposal for next steps with this (I'm fine to merge as-is in case of missing agreement)

HasMissingLicenseTopic(...) is rewritten to be more generic. My ideal solution would be to just loop once and filter the prefixed topics, then only loop through them in a template; or HasTopic(x) returns true/false and doesn't need to be created per topic
cbf- prefix is renamed to something not codeberg-specific, so this patch can be used by other instances and the Codeberg Moderation/Admin tool will be usable without disabling codeberg-specific features like that (because it can be used by every other instance then)

My proposal for next steps with this (I'm fine to merge as-is in case of missing agreement) - HasMissingLicenseTopic(...) is rewritten to be more generic. My ideal solution would be to just loop once and filter the prefixed topics, then only loop through them in a template; or HasTopic(x) returns true/false and doesn't need to be created per topic - `cbf-` prefix is renamed to something not codeberg-specific, so this patch can be used by other instances and the Codeberg Moderation/Admin tool will be usable without disabling codeberg-specific features like that (because it can be used by every other instance then)

👍 1

Gusted commented

2022-11-07 20:43:36 +01:00

cbf- prefix is renamed to something not codeberg-specific, so this patch can be used by other instances and the Codeberg Moderation/Admin tool will be usable without disabling codeberg-specific features like that (because it can be used by every other instance then)

What about internal- or reserved-?

HasMissingLicenseTopic(...) is rewritten to be more generic. My ideal solution would be to just loop once and filter the prefixed topics, then only loop through them in a template; or HasTopic(x) returns true/false and doesn't need to be created per topic

Alright moving to HasTopic(x) interface.

> cbf- prefix is renamed to something not codeberg-specific, so this patch can be used by other instances and the Codeberg Moderation/Admin tool will be usable without disabling codeberg-specific features like that (because it can be used by every other instance then) What about `internal-` or `reserved-`? > HasMissingLicenseTopic(...) is rewritten to be more generic. My ideal solution would be to just loop once and filter the prefixed topics, then only loop through them in a template; or HasTopic(x) returns true/false and doesn't need to be created per topic Alright moving to `HasTopic(x)` interface.

6543 added 1 commit

2022-11-11 16:06:30 +01:00

Merge branch 'codeberg-1.17' into improve-cbf-topics 63c9f8d68b

6543 commented

2022-11-11 16:08:31 +01:00

we might merge it ... as we can itterate on it later

Y/N?

we might merge it ... as we can itterate on it later Y/N?

fnetX commented

2022-11-11 19:12:54 +01:00

@Gusted since there is a limit in length and I prefer if the label names are descriptive, we should choose a short prefix IMHO. It could be "iflag" (internal flag) or we could use some disallowed name if this is not a problem (starting with - for example).

Otherwise, what about 0-- or something like this? (0--abuse ...)

@Gusted since there is a limit in length and I prefer if the label names are descriptive, we should choose a short prefix IMHO. It could be "iflag" (internal flag) or we could use some disallowed name if this is not a problem (starting with - for example). Otherwise, what about 0-- or something like this? (0--abuse ...)

Gusted commented

2022-11-11 23:07:54 +01:00

iflag is fine with me.

fnetX commented

2022-11-11 23:16:49 +01:00

The disadvantage of choosing a term is that the "iflag" project that comes up in two years faces unexpected behaviour if they want to label a repo with iflag-official, iflag-meta and iflag-contribution-welcome ^^

(Feel free to go ahead, anyway, this idea just came to my mind)

The disadvantage of choosing a term is that the "iflag" project that comes up in two years faces unexpected behaviour if they want to label a repo with iflag-official, iflag-meta and iflag-contribution-welcome ^^ (Feel free to go ahead, anyway, this idea just came to my mind)

6543 commented

2023-04-01 16:17:43 +02:00

should I rebase it to v18 ?!?

6543 added 1 commit

2023-04-01 16:17:56 +02:00

Merge branch 'codeberg-1.17' into improve-cbf-topics fc31cfa0de

fnetX commented

2023-04-02 12:03:36 +02:00

Once Gusted has some free cycles, this is probably best forwarded to Forgejo and fixed for the iflag- prefix.

fnetX referenced this pull request from Codeberg/Community

2023-11-28 22:43:19 +01:00

Forgejo December 2023 Priority list #1356