vikunja/vikunja
5
48
Fork 86
Code Issues 150 Pull Requests 36 Packages Releases 37 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 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
fix: default value for migration should be boolean
Some checks failed
continuous-integration/drone/pr Build is failing
Details
1b25245a2d
waza-ari added 1 commit 2024-03-09 21:10:35 +00:00
fix my stupidity and hope that commits will be squashed during pr...
All checks were successful
continuous-integration/drone/pr Build is passing
Details
5975bfd501
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
test: add test to compare getting teams with and without public team feature flag enabled
All checks were successful
continuous-integration/drone/pr Build is passing
Details
8cefe76af5
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
tests: split test to check public team discovery in two runs
All checks were successful
continuous-integration/drone/pr Build is passing
Details
4b72a28f04
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
fix(i18n): rename label for public team checkbox
All checks were successful
continuous-integration/drone/pr Build is passing
Details
8d2f31af02
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
Milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
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?