vikunja
/
vikunja
6
43
Fork 71
Code Issues 150 Pull Requests 17 Packages Releases 32 Activity

feat(teams): add public flags to teams to allow easier sharing with other teams #2179

Merged
konrad merged 16 commits from waza-ari/vikunja:2173-teams-public-flag into main 2024-03-10 14:04:35 +00:00
Conversation 5 Commits 16 Files Changed 22 +392 -45
waza-ari commented 2024-03-09 20:31:09 +00:00
Contributor
Copy Link

Implements #2173

Implements #2173
waza-ari added 8 commits 2024-03-09 20:31:11 +00:00
continuous-integration/drone/pr Build is failing Details
1dfaeaa413 docs: regenerate swagger docs
waza-ari requested review from konrad 2024-03-09 20:31:24 +00:00
waza-ari force-pushed 2173-teams-public-flag from 1dfaeaa413 to 93672cae09 2024-03-09 20:31:36 +00:00 Compare
waza-ari changed title from 2173-teams-public-flag to feat(teams): add public flags to teams to allow easier sharing with other teams 2024-03-09 20:35:20 +00:00
waza-ari added 1 commit 2024-03-09 20:52:31 +00:00
continuous-integration/drone/pr Build is failing Details
1b25245a2d fix: default value for migration should be boolean
waza-ari added 1 commit 2024-03-09 21:10:35 +00:00
frederick commented 2024-03-09 21:17:31 +00:00
Member
Copy Link

Hi waza-ari!

Thank you for creating a PR!

I've deployed the frontend changes of this PR on a preview environment under this URL: https://2179-2173-teams-public-flag--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somewhere. The easiest to use is https://try.vikunja.io/.

This preview does not contain any changes made to the api, only the frontend.

Have a nice day!

Beep boop, I'm a bot.

Hi waza-ari! Thank you for creating a PR! I've deployed the frontend changes of this PR on a preview environment under this URL: https://2179-2173-teams-public-flag--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somewhere. The easiest to use is https://try.vikunja.io/. This preview does not contain any changes made to the api, only the frontend. Have a nice day! > Beep boop, I'm a bot.
waza-ari added 1 commit 2024-03-09 21:39:07 +00:00
waza-ari commented 2024-03-09 22:21:01 +00:00
Author
Contributor
Copy Link

Question: right now (even before this PR), empty groups (no team members) are not returned, as INNER JOIN is used and therefore those are filtered out. Now I'm struggling to think about any scenario where this would be possible, as regular teams cannot be left when there's only a single team member is left and OIDC teams should be auto deleted. So its probably just an issue that could happen with bogus fixture data, just want to confirm that's not an issue?

Question: right now (even before this PR), empty groups (no team members) are not returned, as `INNER JOIN` is used and therefore those are filtered out. Now I'm struggling to think about any scenario where this would be possible, as regular teams cannot be left when there's only a single team member is left and OIDC teams should be auto deleted. So its probably just an issue that could happen with bogus fixture data, just want to confirm that's not an issue?
konrad reviewed 2024-03-10 11:51:59 +00:00
config.yml.sample Outdated
@ -63,2 +63,4 @@
# Allow using a custom logo via external URL.
customlogourl: ''
# Enables the public team feature. If enabled, it is possible to configure teams to be public, which makes them
# discoverable when sharing a project, therefore not only showing teams the user is member of.
konrad commented 2024-03-10 11:44:56 +00:00
Owner
Copy Link

Can you fix the indentation here? That should also fix the generated description in the markdown.

Can you fix the indentation here? That should also fix the generated description in the markdown.
waza-ari marked this conversation as resolved
frontend/src/components/sharing/userTeam.vue
@ -364,0 +368,4 @@
results = await searchService.getAll({}, {s: query, includePublic: true})
} else {
results = await searchService.getAll({}, {s: query})
}
konrad commented 2024-03-10 11:46:36 +00:00
Owner
Copy Link

Nitpick, but please refactor to something like this:

const includePublic = props.shareType === 'team' && configStore.publicTeamsEnabled
const results = await searchService.getAll({}, {s: query, includePublic})
Nitpick, but please refactor to something like this: ```js const includePublic = props.shareType === 'team' && configStore.publicTeamsEnabled const results = await searchService.getAll({}, {s: query, includePublic}) ```
waza-ari commented 2024-03-10 12:35:46 +00:00
Author
Contributor
Copy Link

I was thinking about that as well, but the entire point of having the abstract searchService is that it can be used both for searching teams and users. While the team search service understands the includePublic parameter, the user search service doesn't.

I was thinking about that as well, but the entire point of having the abstract `searchService` is that it can be used both for searching teams and users. While the team search service understands the `includePublic` parameter, the user search service doesn't.
konrad commented 2024-03-10 13:36:24 +00:00
Owner
Copy Link

Gotcha, that makes sense. I stumbled upon this because I forgot the service can be different things.

Gotcha, that makes sense. I stumbled upon this because I forgot the service can be different things.
konrad marked this conversation as resolved
frontend/src/i18n/lang/en.json Outdated
@ -923,1 +923,3 @@
"member": "Member"
"member": "Member",
"isPublic": "Visibility",
"isPublicDescription": "Make team publicly discoverable"
konrad commented 2024-03-10 11:47:20 +00:00
Owner
Copy Link

We should describe what this implies.

We should describe what this implies.
waza-ari marked this conversation as resolved
frontend/src/views/teams/EditTeam.vue Outdated
@ -305,6 +329,8 @@ async function save() {
}
showErrorTeamnameRequired.value = false
console.log('team.value', team.value)
konrad commented 2024-03-10 11:48:00 +00:00
Owner
Copy Link

Please remove the console.log

Please remove the console.log
waza-ari marked this conversation as resolved
pkg/models/teams_test.go Outdated
@ -126,6 +149,50 @@ func TestTeam_ReadAll(t *testing.T) {
assert.Len(t, ts, 1)
assert.Equal(t, int64(2), ts[0].ID)
})
t.Run("public", func(t *testing.T) {
konrad commented 2024-03-10 11:51:14 +00:00
Owner
Copy Link

Please split this up in different test cases (with t.Run)

Please split this up in different test cases (with `t.Run`)
waza-ari marked this conversation as resolved
konrad commented 2024-03-10 11:52:50 +00:00
Owner
Copy Link

Question: right now (even before this PR), empty groups (no team members) are not returned, as INNER JOIN is used and therefore those are filtered out. Now I'm struggling to think about any scenario where this would be possible, as regular teams cannot be left when there's only a single team member is left and OIDC teams should be auto deleted. So its probably just an issue that could happen with bogus fixture data, just want to confirm that's not an issue?

I can't think of any scenario either, a team with no members does not really have a use-case IMHO.

> Question: right now (even before this PR), empty groups (no team members) are not returned, as `INNER JOIN` is used and therefore those are filtered out. Now I'm struggling to think about any scenario where this would be possible, as regular teams cannot be left when there's only a single team member is left and OIDC teams should be auto deleted. So its probably just an issue that could happen with bogus fixture data, just want to confirm that's not an issue? I can't think of any scenario either, a team with no members does not really have a use-case IMHO.
👍 1
waza-ari added 4 commits 2024-03-10 13:03:41 +00:00
waza-ari commented 2024-03-10 13:32:13 +00:00
Author
Contributor
Copy Link

Hm, I'm uncertain if my changes could cause CI to fail, not sure if anything else was changed?

Hm, I'm uncertain if my changes could cause CI to fail, not sure if anything else was changed?
konrad commented 2024-03-10 13:37:08 +00:00
Owner
Copy Link

The desktop build is sometimes flaky, no worries. I've restarted that CI build.

The desktop build is sometimes flaky, no worries. I've restarted that CI build.
konrad reviewed 2024-03-10 13:45:16 +00:00
frontend/src/i18n/lang/en.json Outdated
@ -922,2 +922,3 @@
"admin": "Admin",
"member": "Member"
"member": "Member",
"isPublic": "Visibility",
konrad commented 2024-03-10 13:45:16 +00:00
Owner
Copy Link

This should be called "Public Team" or something similar, calling it "Visibility" does not really say what it does. In combination with the explanation "Public Team" makes more sense.

This should be called "Public Team" or something similar, calling it "Visibility" does not really say what it does. In combination with the explanation "Public Team" makes more sense.
konrad marked this conversation as resolved
waza-ari added 1 commit 2024-03-10 13:46:45 +00:00
continuous-integration/drone/pr Build is passing Details
8d2f31af02 fix(i18n): rename label for public team checkbox
konrad approved these changes 2024-03-10 13:49:02 +00:00
konrad left a comment
Owner
Copy Link

LGTM. Thanks!

LGTM. Thanks!
konrad scheduled this pull request to auto merge when all checks succeed 2024-03-10 13:49:19 +00:00
konrad merged commit ffa82556e0 into main 2024-03-10 14:04:35 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
konrad
Labels
Clear labels   
dependencies

   
duplicate

This issue or pull request already exists

  
help wanted

Need some help

  
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
needs reproduction

   
question

More information is needed

  
security

   
wontfix

This won't be fixed

No Label
dependencies
duplicate
help wanted
invalid
kind/bug
kind/feature
needs reproduction
question
security
wontfix
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/vikunja#2179
No description provided.
Delete Branch "waza-ari/vikunja:2173-teams-public-flag"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?