[Feature] Improve the search_owner when query the app list #1828
Conversation
|
@1996fanrui PTAL |
|
Hi @wolfboys , I have a question. There are some applications belong to admin. Should we show all app owner for current team?
|
In other word, if user_a is admin, he doesn't belong to team1, and he has some apps in team1, should we show the user_a here? If user_b isn't admin. he belongs to team1. He doesn't have any apps. Should we show user_b here? |
if usertype is admin, the range of users: Himself + members of the team
yes |
|
hi fanrui:
if usertype is admin, the range of users: Himself (has app) + members of the team (members has least one app)
yes, but user_b must be has least one app, else no. |
| select u.* from t_user u | ||
| inner join ( | ||
| select distinct(user_id) as user_id from ( | ||
| select a.user_id | ||
| from t_user a | ||
| inner join t_flink_app b | ||
| on a.user_id = b.user_id | ||
| where a.user_id = #{userId} | ||
| and a.user_type = 1 | ||
| union all | ||
| select u.user_id | ||
| from t_user u | ||
| inner join t_member m | ||
| on u.user_id = m.user_id | ||
| inner join t_flink_app p | ||
| on u.user_id = p.user_id | ||
| where m.team_id = #{teamId} | ||
| ) | ||
| ) as t | ||
| where u.user_id = t.user_id |
There was a problem hiding this comment.
Why don't use this sql?
select u.* from t_user u
inner join (
select distinct(user_id) from t_flink_app where m.team_id = #{teamId}
) t
where u.user_id = t.user_id
The new sql has some advantages:
- It is more simple.
- It is easy to understand for other developers.
- And it can show all apps owner for current team.
- When some team members leave or move to other teams, we should also be able to filter his apps.
- For the System Admin team, Admins often have some test jobs. AdminA should be able to filter AdminB's apps.
The new sql is simple and feature rich, why use a complex sql?
There was a problem hiding this comment.
Why don't use this sql?
select u.* from t_user u inner join ( select distinct(user_id) from t_flink_app where m.team_id = #{teamId} ) t where u.user_id = t.user_idThe new sql has some advantages:
- It is more simple.
- It is easy to understand for other developers.
- And it can show all apps owner for current team.
- When some team members leave or move to other teams, we should also be able to filter his apps.
- For the System Admin team, Admins often have some test jobs. AdminA should be able to filter AdminB's apps.
The new sql is simple and feature rich, why use a complex sql?
oh, Perfect 👍 😂
There was a problem hiding this comment.
Hi @wolfboys , LGTM, thanks for your great contribution.
Hi @lvshaokang @MonsterChenzhuo , please help double check, thanks.
| @@ -102,4 +102,9 @@ public class User implements Serializable { | |||
| */ | |||
| private Long teamId; | |||
|
|
|||
| public void dataMasking() { | |||
| this.setPassword("******"); | |||
There was a problem hiding this comment.
hi， @wolfboys Thank you for your contribution。Strings with special meanings are recommended to be extracted as public variables，do you think？
There was a problem hiding this comment.
hi， @wolfboys Thank you for your contribution。Strings with special meanings are recommended to be extracted as public variables，do you think？
Great suggestion, I agree with you. I'll fix it later
[Feature] Improve the search_owner when query the app list
Contribution Checklist
If this is your first time, please read our contributor guidelines: Submit Code.
Make sure that the pull request corresponds to a GITHUB issue.
Name the pull request in the form "[Feature] Title of the pull request", where Feature can be replaced by
Hotfix,Bug, etc.Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
If the PR is unfinished, add
[WIP]in your PR title, e.g.,[WIP][Feature] Title of the pull request.-->
What changes were proposed in this pull request
Issue Number: close #1827
Brief change log
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts