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

feat: add projects select to analytics page gf-348 #470

Merged
merged 18 commits into from
Sep 24, 2024

Conversation

AnutkaGn
Copy link
Collaborator

Add project selector to the analytics page
When the project is selected only contributors from this project be shown.
image

@AnutkaGn
Copy link
Collaborator Author

Can I make a separate request to get all the projects, because it is a bit problematic to make it an optional query? Because I need to extract all the projects for the selector

@@ -227,17 +227,11 @@ class ProjectController extends BaseController {
*/
private async findAll(
options: APIHandlerOptions<{
query: ProjectGetAllRequestDto;
query: object | ProjectGetAllRequestDto;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
query: object | ProjectGetAllRequestDto;
query: ProjectGetAllRequestDto;

specify type

@@ -120,7 +120,7 @@ class ProjectService implements Service {
}

public async findAll(
parameters: ProjectGetAllRequestDto,
parameters: object | ProjectGetAllRequestDto,
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@liza-veis
Copy link
Collaborator

Pay attention to the changes in this PR #468, so that they are not conflicting

@AnutkaGn AnutkaGn requested a review from what1s1ove September 23, 2024 07:50
Copy link
Collaborator

Choose a reason for hiding this comment

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

Place helpers in their separate folders

const { page } = action.meta.arg;

state.projects =
const page = action.meta.arg?.page;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const page = action.meta.arg?.page;
const { page } = action.meta.arg;

@github-actions github-actions bot removed the shared label Sep 23, 2024
const { projects } = useAppSelector(({ projects }) => projects);

useEffect(() => {
void dispatch(projectActions.loadAll());
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to load these projects separately, specifically for analytics module, so it should be in analytics slice. Also on the backend side we need to split service method for pagination and without and the same for repository. Take a look at how it's done here #491

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did I understand correctly that I can make a separate request on the backend in order to get all the projects (without pagination)? And use it in the analytics slice? I thought it would be better that way, I asked, but I didn't get an answer then

Copy link
Collaborator

@liza-veis liza-veis Sep 23, 2024

Choose a reason for hiding this comment

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

Yes, let's make it as a separate request and keep it separately

Copy link
Collaborator

Choose a reason for hiding this comment

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

probably missed your question in the chat

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Everything is fine, I hope it is correct now

…4-gitfit into 348-feat-add-projects-select-to-analytics-page
…4-gitfit into 348-feat-add-projects-select-to-analytics-page
@@ -1,5 +1,6 @@
const ProjectsApiPath = {
$ID: "/:id",
All: "/all",
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's use the same route, but call different method on the backend depending on the query parameters, If we pass page, then call method with pagination, otherwise another method

Suggested change
All: "/all",

Comment on lines 276 to 280
const projects = await this.projectService.findAllWithoutPagination();

return {
payload: projects,
status: HTTPCode.OK,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const projects = await this.projectService.findAllWithoutPagination();
return {
payload: projects,
status: HTTPCode.OK,
return {
payload: await this.projectService.findAllWithoutPagination(),
status: HTTPCode.OK,

Comment on lines 143 to 147
return projects.map((project) => {
const { id, lastActivityDate, name } = project.toObject();

return { id, lastActivityDate, name };
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we do it like

Suggested change
return projects.map((project) => {
const { id, lastActivityDate, name } = project.toObject();
return { id, lastActivityDate, name };
});
return projects.map((project) => project.toObject());

@@ -4,4 +4,5 @@ export {
type ActivityLogGetAllItemResponseDto,
type ActivityLogGetAllResponseDto,
type ActivityLogQueryParameters,
type ProjectGetAllItemResponseDto,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure, but maybe it'll be better to import this type from ~/modules/projects/projects.ts in places where you need it, and not reexport this here?

@github-actions github-actions bot removed the shared label Sep 23, 2024
@AnutkaGn AnutkaGn requested a review from liza-veis September 23, 2024 19:32
@liza-veis liza-veis merged commit 47d8620 into main Sep 24, 2024
6 checks passed
This was referenced Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

5 participants