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

fix(teams): fix duplicate teams being shown when new public team visibility feature is enabled #2187

Merged
konrad merged 2 commits from waza-ari/vikunja:fix-duplicate-teams-when-sharing into main 2024-03-10 21:42:36 +00:00
Conversation 4 Commits 2 Files Changed 1 +1 -1
waza-ari commented 2024-03-10 15:06:11 +00:00
Contributor
Copy Link

Sorry, missed another case that I only noticed after updating unstable in our environment... Due to the INNER JOIN on the team_members table and the new OR conditions allowing teams with the isPublic flag set to true, teams are returned multiple times. As we're only after the teams, a simple distinct query should fix the issue.

Sorry, missed another case that I only noticed after updating unstable in our environment... Due to the `INNER JOIN` on the `team_members` table and the new `OR` conditions allowing teams with the `isPublic` flag set to `true`, teams are returned multiple times. As we're only after the teams, a simple distinct query should fix the issue.
waza-ari added 1 commit 2024-03-10 15:06:13 +00:00
continuous-integration/drone/pr Build is passing Details
53631f5d09 fix(teams): fix duplicate teams being shown when
frederick commented 2024-03-10 15:13:05 +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://2187-fix-duplicate-teams-when-sharing--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://2187-fix-duplicate-teams-when-sharing--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.
konrad reviewed 2024-03-10 15:35:08 +00:00
pkg/models/teams.go Outdated
@ -309,3 +309,3 @@
builder.Eq{"team_members.user_id": a.GetID()},
),
)
).Distinct("teams.id")
konrad commented 2024-03-10 15:35:08 +00:00
Owner
Copy Link

Doesn't this either return only team ids or is not taken into account at all?

Doesn't this either return only team ids or is not taken into account at all?
waza-ari commented 2024-03-10 15:52:08 +00:00
Author
Contributor
Copy Link

With native SQL I'd completely agree with you. Not entirely sure why, but I compared the SQL command executed before and after the change:

Before:

SELECT teams.* FROM `teams` INNER JOIN `team_members` ON team_members.team_id = teams.id WHERE teams.name LIKE ? AND (teams.is_public=? OR team_members.user_id=?) LIMIT 50

After:

SELECT DISTINCT teams.* FROM `teams` INNER JOIN `team_members` ON team_members.team_id = teams.id WHERE teams.name LIKE ? AND (teams.is_public=? OR team_members.user_id=?) LIMIT 50

Not entire sure why this is happening to be honest. Changing the code to

Distinct("teams.*")

has the same effect:

SELECT DISTINCT teams.* FROM `teams` INNER JOIN `team_members` ON team_members.team_id = teams.id WHERE teams.name LIKE ? AND (teams.is_public=? OR team_members.user_id=?) LIMIT 50
With native SQL I'd completely agree with you. Not entirely sure why, but I compared the SQL command executed before and after the change: Before: ```sql SELECT teams.* FROM `teams` INNER JOIN `team_members` ON team_members.team_id = teams.id WHERE teams.name LIKE ? AND (teams.is_public=? OR team_members.user_id=?) LIMIT 50 ``` After: ```sql SELECT DISTINCT teams.* FROM `teams` INNER JOIN `team_members` ON team_members.team_id = teams.id WHERE teams.name LIKE ? AND (teams.is_public=? OR team_members.user_id=?) LIMIT 50 ``` Not entire sure why this is happening to be honest. Changing the code to ```go Distinct("teams.*") ``` has the same effect: ```sql SELECT DISTINCT teams.* FROM `teams` INNER JOIN `team_members` ON team_members.team_id = teams.id WHERE teams.name LIKE ? AND (teams.is_public=? OR team_members.user_id=?) LIMIT 50 ```
konrad commented 2024-03-10 17:45:04 +00:00
Owner
Copy Link

My guess would be the .Select() overrides the .Distinct.

Can you move the Distinct to the main part of the query and combine it with the Select? Doesn't hurt to have it there always.

My guess would be the `.Select()` overrides the `.Distinct`. Can you move the `Distinct` to the main part of the query and combine it with the `Select`? Doesn't hurt to have it there always.
waza-ari commented 2024-03-10 17:51:36 +00:00
Author
Contributor
Copy Link

I've been reading a bit and it looks like you're right. You don't even need select, you can call Distinct directly on the engine which is automatically adding the SELECT. Changed it accordingly.

I've been reading a bit and it looks like you're right. You don't even need `select`, you can call `Distinct` [directly on the engine](https://xorm.io/docs/chapter-05/1.conditions/) which is automatically adding the `SELECT`. Changed it accordingly.
konrad commented 2024-03-10 15:35:26 +00:00
Owner
Copy Link

How can I reproduce the bug you're describing?

How can I reproduce the bug you're describing?
waza-ari commented 2024-03-10 15:42:59 +00:00
Author
Contributor
Copy Link

How can I reproduce the bug you're describing?

You'll need two users and two teams at the very least:

Teams:

  • Team A
  • Team B

Users:

  • User A: Member of Team A and Team B
  • User B: Member of Team B

User B then creates a new project and shares it. Team B will be returned twice

> How can I reproduce the bug you're describing? You'll need two users and two teams at the very least: Teams: - Team A - Team B Users: - User A: Member of Team A and Team B - User B: Member of Team B User B then creates a new project and shares it. Team B will be returned twice
waza-ari added 1 commit 2024-03-10 17:49:46 +00:00
continuous-integration/drone/pr Build is passing Details
7c5bca08ce fix: replace select with distinct instead of adding it
waza-ari commented 2024-03-10 21:12:37 +00:00
Author
Contributor
Copy Link

@konrad I don't think the CI failure is caused by the change - could you trigger the failed job again please?

@konrad I don't think the CI failure is caused by the change - could you trigger the failed job again please?
konrad merged commit 6c98052176 into main 2024-03-10 21:42:36 +00:00
konrad deleted branch fix-duplicate-teams-when-sharing 2024-03-10 21:42:37 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
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#2187
No description provided.
Delete Branch "waza-ari/vikunja:fix-duplicate-teams-when-sharing"

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?