Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect issue filter in user dashboard #23630

Closed
wants to merge 14 commits into from

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented Mar 22, 2023

Fixes #22799

Changes:

  • Fixed logic problems when filtering issues.
    • Avoid displaying all issues of the repo which user only created an issues/be mentioned/be assigned
    • Team permission doesn't work
  • Display all issues count info in repo filter to avoid displaying strange ui as following:
    image
    (selected a repo in repo filter which have no issues created by user, then access created by you)
    After fix:
    image

@yp05327 yp05327 force-pushed the fix-issues-in-user-dashboard branch from f087fa3 to 313520e Compare March 23, 2023 09:01
@yp05327 yp05327 force-pushed the fix-issues-in-user-dashboard branch from 313520e to 5343ec1 Compare March 24, 2023 00:43
@yp05327 yp05327 changed the title [WIP] Fix incorrect issue filter in user dashboard Fix incorrect issue filter in user dashboard Mar 24, 2023
@@ -444,21 +444,6 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
repoOpts.TeamID = team.ID
}

switch filterMode {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether we need to remove repoOpts above in this PR. it is not used.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 24, 2023
@@ -1402,17 +1404,64 @@ func issuePullAccessibleRepoCond(repoIDstr string, userID int64, org *organizati
} else {
cond = cond.And(
builder.Or(
repo_model.UserOwnedRepoCond(userID), // owned repos
repo_model.UserAccessRepoCond(repoIDstr, userID), // user can access repo in a unit independent way
repo_model.UserAssignedRepoCond(repoIDstr, userID), // user has been assigned accessible public repos
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repo_model.UserAssignedRepoCond is only used here.
Do I need to remove it or keep it.

repo_model.UserOwnedRepoCond(userID), // owned repos
repo_model.UserAccessRepoCond(repoIDstr, userID), // user can access repo in a unit independent way
repo_model.UserAssignedRepoCond(repoIDstr, userID), // user has been assigned accessible public repos
repo_model.UserMentionedRepoCond(repoIDstr, userID), // user has been mentioned accessible public repos
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

repo_model.UserAccessRepoCond(repoIDstr, userID), // user can access repo in a unit independent way
repo_model.UserAssignedRepoCond(repoIDstr, userID), // user has been assigned accessible public repos
repo_model.UserMentionedRepoCond(repoIDstr, userID), // user has been mentioned accessible public repos
repo_model.UserCreateIssueRepoCond(repoIDstr, userID, isPull), // user has created issue/pr accessible public repos
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

@yp05327 yp05327 force-pushed the fix-issues-in-user-dashboard branch from ed31247 to da6e7f8 Compare March 24, 2023 06:03
jolheiser pushed a commit that referenced this pull request Apr 3, 2023
All `access_mode` value of Owner Teams are 0(AccessModeNone) in
`team_unit` table, which should be 4(AccessModeOwner)
In `team` table:

![image](https://user-images.githubusercontent.com/18380374/227409457-1b9660ae-8cf7-49c8-a013-1850b46baebc.png)
In `team_unit` table:

![image](https://user-images.githubusercontent.com/18380374/227409429-a793dd90-4ae1-4191-b95b-e288c591f9fd.png)

ps: In #23630, `access_mode` in
`team_unit` is used to check the team unit permission, but I found that
user can not see issues in owned org repos.
@yp05327
Copy link
Contributor Author

yp05327 commented Apr 20, 2023

Got the sql in mssql. Have no idea with the error.

sql
SELECT
    issue.repo_id AS repo_id,
    COUNT(*) AS count
FROM
    [issue]
    INNER JOIN
        [repository]
    ON  [issue].repo_id = [repository].id
WHERE
    (
        issue.is_closed = ?
    )
AND (
        issue.is_pull = ?
    )
AND repository.is_archived = ?
AND (
        repository.owner_id = ?
    OR  issue.repo_id IN(
            SELECT
                [access].repo_id
            FROM
                [access]
                INNER JOIN
                    repository
                ON  [repository].id = [access].repo_id
                INNER JOIN
                    user
                ON  [user].id = [repository].owner_id
            WHERE
                [access].user_id = ?
            AND [access].mode > ?
            AND (
                    (
                        [user].type = ?
                    AND (
                            [access].repo_id IN(
                                SELECT
                                    [team_repo].repo_id
                                FROM
                                    team_repo
                                    INNER JOIN
                                        team_user
                                    ON  [team_user].team_id = [team_repo].team_id
                                    INNER JOIN
                                        team_unit
                                    ON  [team_unit].team_id = [team_repo].team_id
                                WHERE
                                    [team_user].uid = ?
                                AND [team_unit].[type] = ?
                                AND [team_unit].[access_mode] > ?
                            )
                        OR  [access].repo_id IN(
                                SELECT
                                    repo_id
                                FROM
                                    [collaboration]
                                WHERE
                                    [collaboration].user_id = ?
                            )
                        )
                    )
                OR  [user].type = ?
                )
        )
    OR  (
            repository.is_private = ?
        AND issue.id IN(
                SELECT
                    issue_assignees.issue_id
                FROM
                    issue_assignees
                WHERE
                    issue_assignees.assignee_id = ?
            )
        )
    OR  (
            repository.is_private = ?
        AND issue.id IN(
                SELECT
                    issue_user.issue_id
                FROM
                    issue_user
                WHERE
                    issue_user.is_mentioned = ?
                AND issue_user.uid = ?
            )
        )
    OR  (
            repository.is_private = ?
        AND issue.id IN(
                SELECT
                    issue.id
                FROM
                    issue
                WHERE
                    issue.is_pull = ?
                AND issue.poster_id = ?
            )
        )
    )
GROUP BY
    issue.repo_id 

@wxiaoguang
Copy link
Contributor

Try to quote all names, like user, mode, type.

@yp05327 yp05327 force-pushed the fix-issues-in-user-dashboard branch from ff4432a to 862c853 Compare April 20, 2023 05:21
@wxiaoguang
Copy link
Contributor

Hmm, CI passes. That's why I recommend to use two words for variable names / table names / column names @lunny It saves a lot of debugging time.

ps: the SQL seems quite complex ......

@lunny
Copy link
Member

lunny commented Apr 20, 2023

Hmm, CI passes. That's why I recommend to use two words for variable names / table names / column names @lunny It saves a lot of debugging time.

ps: the SQL seems quite complex ......

Yes, too complex.

@yp05327
Copy link
Contributor Author

yp05327 commented Apr 20, 2023

It works now.
Then the next step is improving the SQL. 😄

ps: maybe we should also add some new tests.

@silverwind
Copy link
Member

Should re-test, I recall @lunny made some recent changes surrounding filters that may impact this.

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 22, 2023
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 22, 2023
@yp05327
Copy link
Contributor Author

yp05327 commented Jul 21, 2023

Simplified the SQL in UserUnitAccessRepoCond, as I found AccessibleRepositoryCondition function which is what I need.

}),
),
)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between these functions and the ones in repo_model? Maybe outline in a comment the differences?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functions in repo_model are only used here (but will cause the issue). So we can remove them or move these new functions into repo_model. I didn't do that because I'm not sure whether the functions in repo_model are only designed for issuePullAccessibleRepoCond.

The difference is that the functions in repo_model use issue.repo_id in repo_ids to filter the issue, so all issues in these repos will be caught.
So we need use issue.id in issue_ids to get the correct issues.

Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try it I guess.

@GiteaBot GiteaBot 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 Aug 20, 2023
@lunny
Copy link
Member

lunny commented Aug 23, 2023

I think this has been replaced by #26657 ?

@denyskon
Copy link
Member

@yp05327 What do you think, can this PR be closed?

@denyskon denyskon added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Sep 10, 2023
@yp05327
Copy link
Contributor Author

yp05327 commented Sep 11, 2023

After update go version to 1.21.0 in my dev container, I can't run Gitea correctly now, so it will take some times for me to confirm this. 😕
image

@yp05327
Copy link
Contributor Author

yp05327 commented Sep 11, 2023

Got it work!
Then it seems that there are some new problems. I can not see some issues created by the user:
(these two screenshots come from Testuser)
image
image

@yp05327
Copy link
Contributor Author

yp05327 commented Sep 11, 2023

As we have rewrite the search engine for issues, I will close this PR.
But there are still some issues now, and I think we need to add more CI tests.

@yp05327 yp05327 closed this Sep 11, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail lgtm/need 1 This PR needs approval from one additional maintainer to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wrong repo filter in user PR page?
6 participants