Added teams functionality #380
Conversation
AddTeamToProjectAdapter
CreateTeamAdapter
DeleteTeamAdapter
ListProjectsForTeamAdapter
ListTeamsForUserAdapter
ListUsersAdapter
TeamMapper
and applied spotless.
…ent ListTeamsService
Done |
…d-negative-tests
Add negative tests
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| coderadarConfigurationProperties.setAuthentication( |
There was a problem hiding this comment.
As far as I know, til now, this is the only test using such a configuration. if there are more configurations like this, we should use a spring profile for tests to handle these configurations for us.
There was a problem hiding this comment.
I'm not sure what you mean by configuration. The credentials are basically mocked in these tests.
There was a problem hiding this comment.
I thought this line would basically just set the property coderadar.authentication.enabled=true of he local.application.properties file. This was a misunderstanding by me.
There was a problem hiding this comment.
That is correct, however I did not undestand the issue.
| testUser.setProjects(Collections.singletonList(testProject)); | ||
| userRepository.save(testUser); | ||
| } | ||
|
|
There was a problem hiding this comment.
What happens if there is no user with this id for this project? What happens if user and project do not exist?
There was a problem hiding this comment.
I'm not sure what you mean. The cases you describe are tested on lines 77 and 94.
There was a problem hiding this comment.
The first means the user exists (has id 1) but he is not a user registered for this project. The test case you've used in line 77 is for a user which does not exist on our platform. For the second part, this file only does have 87 lines, at least in the GitHub view. What test is written in line 94?
There was a problem hiding this comment.
Sorry, the lines I mentioned are for the SetUserRoleForProjectIntegrationTest. The first test checks if a user assigned to a project gets their role removed successfuly. The other two tests check that 404 is returned when a user/project does not exists. Is there something I'm missing?
| testUser.setPassword(PasswordUtil.hash("password1")); | ||
| userRepository.save(testUser); | ||
| } | ||
|
|
There was a problem hiding this comment.
What happens if there is no user with this id for this project? What happens if user and project do not exist?
coderadar-ui/src/app/app.module.ts
Outdated
| import {ContributorMergeDialogComponent} from './components/contributor-merge-dialog/contributor-merge-dialog.component'; | ||
| import {DeleteProjectDialogComponent} from './components/delete-project-dialog/delete-project-dialog.component'; | ||
| import {AddProjectToTeamDialogComponent} from './components/add-project-to-team-dialog/add-project-to-team-dialog.component'; | ||
| import {ContributorDialogComponent} from "./components/contributor-card/contributor-dialog.component"; |
There was a problem hiding this comment.
Please execute TSLint. It should emphasize doulbe quotes in ts files and more. TSLint issues should be fixed.
There was a problem hiding this comment.
Ts lint returns no errors for me. In fact running tslint --fix will remove all double quotes as those are counted as errors.
EDIT: I see the double quote now, I don't know why it was left out as I ran the linter last time. I'll fix it again.
EDIT2: Should we maybe make the linting a part of the build like it is for spotless?
There was a problem hiding this comment.
I've run the linter again
There was a problem hiding this comment.
Thanks, I think it would be a good idea to make it part of the build process!
| @@ -214,7 +203,7 @@ export class ProjectService { | |||
| * @param branch The branch to use for getting the commits. | |||
| */ | |||
| public getCommitsForContributor(id: number, branch: string, email: string): Promise<HttpResponse<Commit[]>> { | |||
| return this.httpClient.get<Commit[]>(this.apiURL + 'projects/' + id + '/' + branch + '/commits?email='+email, | |||
| return this.httpClient.get<Commit[]>(this.apiURL + 'projects/' + id + '/' + branch + '/commits?email=' + email.replace('+', '%2B'), | |||
There was a problem hiding this comment.
Why do you replace all '+' in generated url with '%2B'? '+' has to be encoded, but I don't see any place where a '+' could be added to the url.
Also, if the urls have multiple parameter, we may consider using string literals to increase readability. That would be the scope of a new issue though.
There was a problem hiding this comment.
The + sign is an edge case here. Some emails (those generate by GitHub for example) contain a plus sign. I agree about the string literals.
|
@jo2 Do you want any more changes to be made in this PR? |
|
What I'm stil missing for the |
… to remove the role and write a test for this case
|
Yes, you were correct in that this case wasn't handled. I've added a check for it and wrote a test. |
|
Kudos, SonarCloud Quality Gate passed!
|
Coderadar now provides functionality for adding and managing teams. Each team can be assigned to many projects and with different roles. Users now also have a role for any project that they have access to. Basic authentication and UI elements have also been added. Lots of works still needs to be done in the UI (and the Back-End will likely have to be adjusted), but this can be done later after we have discussed some of the details about the authentication concept and the UI elements.