Refactor projects page #211
Conversation
Refactored to use component but page contents are not added yet
|
|
||
| const fetchProjects = async () => { | ||
| const res = await projects.get(); | ||
| if (res.status == 200) { |
There was a problem hiding this comment.
use then and catch instead of checking the status.
There was a problem hiding this comment.
I think we don't need this condition. Because anyway these statuses are handled in the registerIntercepts. You can enclose the whole code in a try..catch block.
| </div> | ||
| </div> | ||
| {/* {showEditDialog ? ( | ||
| <EditProject |
There was a problem hiding this comment.
why commented code? please remove.
There was a problem hiding this comment.
I will uncomment it when I add related modals
|
|
||
| const fetchProjects = async () => { | ||
| const res = await projects.get(); | ||
| if (res.status == 200) { |
There was a problem hiding this comment.
I think we don't need this condition. Because anyway these statuses are handled in the registerIntercepts. You can enclose the whole code in a try..catch block.
| @@ -0,0 +1,81 @@ | |||
| import * as React from "react"; | |||
| import { minutesToHHMM } from "../../helpers/hhmm-parser"; | |||
There was a problem hiding this comment.
| import { minutesToHHMM } from "../../helpers/hhmm-parser"; | |
| import { minutesToHHMM } from "helpers/hhmm-parser"; |
| def projects | ||
| projects = current_user.current_workspace.projects | ||
| projects.kept.map { | project | | ||
| { id: project.id, | ||
| name: project.name, | ||
| clientName: project.client.name, | ||
| isBillable: project.billable, | ||
| minutesSpent: project.total_hours_logged } } | ||
| end |
There was a problem hiding this comment.
Please move this method to private. Also try to memoize
app/models/project.rb
Outdated
| def total_hours_logged(time_frame = "week") | ||
| from, to = week_month_year(time_frame) | ||
| timesheet_entries.where(work_date: from..to).sum(:duration) | ||
| end | ||
|
|
||
| # Move weeK_month_year method copied from client.rb to common place | ||
| def week_month_year(time_frame) | ||
| case time_frame | ||
| when "last_week" | ||
| return ((Date.today.beginning_of_week) - 7), ((Date.today.end_of_week) - 7) | ||
| when "month" | ||
| return Date.today.beginning_of_month, Date.today.end_of_month | ||
| when "year" | ||
| return Date.today.beginning_of_year, Date.today.end_of_year | ||
| else | ||
| return Date.today.beginning_of_week, Date.today.end_of_week | ||
| end | ||
| end |
There was a problem hiding this comment.
I think we are using the same method in some other model. Can we move this to a common concern
There was a problem hiding this comment.
Yeah, I have added the same in the comment. where should we move this to? like what is the place for common functionality.
Also added state to check if project is clicked for showing project details
| @@ -4,8 +4,7 @@ class ProjectsController < ApplicationController | |||
| skip_after_action :verify_authorized | |||
|
|
|||
| def index | |||
| @query = Project.ransack(params[:q]) | |||
| @projects = @query.result(distinct: true) | |||
| authorize :project | |||
There was a problem hiding this comment.
This can be authorize Project to denote that the policy belongs to the Project model. We should use symbols for policies when there is no associated model with them
Shows the project basic details and the project member details
Current Code Coverage Percent of this PR:78.53 %Files having coverage below 100%
|
|
@shalapatil Please remove commented code. |
app/javascript/src/apis/projects.ts
Outdated
|
|
||
| const show = async id => axios.get(`${path}/${id}`); | ||
|
|
||
| const projects = { get, show }; |
There was a problem hiding this comment.
| const projects = { get, show }; | |
| const projectApi = { get, show }; |
There was a problem hiding this comment.
for all the API's we can use these pattern
app/javascript/src/apis/projects.ts
Outdated
|
|
||
| const projects = { get, show }; | ||
|
|
||
| export default projects; |
There was a problem hiding this comment.
| export default projects; | |
| export default projectApi; |
| const fetchProjects = () => { | ||
|
|
||
| projects.get() | ||
| .then(res => setAllProjects(res.data.projects)) | ||
| .catch(err => console.log(err)); | ||
|
|
||
| }; |
There was a problem hiding this comment.
| const fetchProjects = () => { | |
| projects.get() | |
| .then(res => setAllProjects(res.data.projects)) | |
| .catch(err => console.log(err)); | |
| }; | |
| const fetchProjects = async () => { | |
| try { | |
| const response = await projects.get(); | |
| setProjects(response.data.projects); | |
| } catch(err => console.log(err)); | |
| }; |
There was a problem hiding this comment.
what's the difference?
| import ProjectList from "./projectList"; | ||
|
|
||
| const Projects = ({ editIcon, deleteIcon, isAdminUser }) => { | ||
| const [allProjects, setAllProjects] = React.useState([]); |
There was a problem hiding this comment.
| const [allProjects, setAllProjects] = React.useState([]); | |
| const [projects, setProjects] = React.useState<ProjectsData[]>([]); |
There was a problem hiding this comment.
We can call the state as Projects.
There was a problem hiding this comment.
Also, add an interface for ProjectsData
| const fetchProject = () => { | ||
| projectsAPI.show(id) | ||
| .then(res => setProject(res.data.project)) | ||
| .catch (err => {console.log(err);}); | ||
| }; |
There was a problem hiding this comment.
Use async await method. refer the above comment
| import { setAuthHeaders, registerIntercepts } from "apis/axios"; | ||
| import projectsAPI from "apis/projects"; | ||
| import { Member } from "./member"; | ||
| import { minutesToHHMM } from "../../helpers/hhmm-parser"; |
| @@ -0,0 +1,90 @@ | |||
| import * as React from "react"; | |||
| import { setAuthHeaders, registerIntercepts } from "apis/axios"; | |||
| import projectsAPI from "apis/projects"; | |||
There was a problem hiding this comment.
| import projectsAPI from "apis/projects"; | |
| import projectAPI from "apis/projects"; |
| }, []); | ||
|
|
||
| return ( | ||
|
|
| projects.kept.map { | project | | ||
| { id: project.id, | ||
| name: project.name, | ||
| clientName: project.client.name, | ||
| isBillable: project.billable, | ||
| minutesSpent: project.total_hours_logged } } |
There was a problem hiding this comment.
This part should be moved to the individual controller action, so that def projects is intended to do only one thing and is reusable. Not a blocking change though.
| @@ -3,7 +3,10 @@ | |||
| json.project_details do | |||
There was a problem hiding this comment.
You can add the following just above this line to auto convert the keys in response to camelCase
json.key_format! camelize: :lower
json.deep_format_keys!
You won't need to modify the keys on FE side after this. For example, no need to change isBillable to is_billable in React code.
There was a problem hiding this comment.
okay, don't have any of the you specified requirement atm. Will add it whenver required.
| end | ||
| json.isBillable project.billable | ||
| json.minutesSpent project.total_hours_logged | ||
| end |
There was a problem hiding this comment.
Could you try
json.key_format! camelize: :lower
json.deep_format_keys!
json.projects do
json.array! projects do |project|
json.id project.id
json.name project.name
json.client do
json.name project.client.name
end
json.is_billable project.billable
json.minutes_spent project.total_hours_logged
end
end
?
| project_members = project.project_member_details.map { | member | | ||
| { name: member[:name], | ||
| hourlyRate: member[:hourly_rate], | ||
| minutesSpent: member[:minutes_spent], | ||
| cost: member[:hourly_rate] * member[:minutes_spent] / 60 }} | ||
| { id: project.id, | ||
| name: project.name, | ||
| clientName: project.client.name, | ||
| isBillable: project.billable, | ||
| minutesSpent: project.total_hours_logged, | ||
| projectMembers: project_members } |
There was a problem hiding this comment.
Why this is not updated?
Notion card
Summary
Refactored projects index page to use react instead of erb file. This is very basic page. All the required components of the page are not added. Someone from FE needs to work on it.
Preview
Expected view
Current implemented view
Clicking on any project, takes to this page
Type of change
Please delete options that are not relevant.
not work as expected)
How Has This Been Tested?
Checklist: