-
Notifications
You must be signed in to change notification settings - Fork 455
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/Added Job Management Page #300
FEAT/Added Job Management Page #300
Conversation
Nice pr, can you fix the conflicts and give a video of your functioning |
ok |
100xJobs.-.Google.Chrome.2024-09-17.13-40-44.mp4 |
@VineeTagarwaL-code solved conflicts, Fixed UI bug( JOB was being displayed even after deleting until we perform hard reload). tag me once you review this. |
this looks great now, a good starting point can you one more feature where the admin can approve jobs will be really usefull |
Yes i can do that, but since there is no session logic, Signup,Register how can i determine logged in User is Admin or Not,? Why there is no signin/signup options? |
can you add that feature here in this PR itself, was gonna open a new one but if you can do it it will be great ! |
sure i'll take this. |
yeah it already there the code is not purged |
summary: Demo.mp4commit these? |
show auth flow with logout |
sorry for keep this hanging , i will get it done by today or tomorrow |
100xJobs.-.Google.Chrome.2024-09-22.10-56-59.mp4 |
We can raise issue for better login and logout button and signin/signup form |
comment out the login flow, only ahve for admin a lot of pr has this and they are more better on this tbh |
100xJobs.-.Google.Chrome.2024-09-22.16-53-40.mp4ready to merge i think |
fix conflicts, need to merge |
fixing |
build fails, fix please |
fixed |
it should work now |
src/actions/job.action.ts
Outdated
>(async (data) => { | ||
const result = deleteJobByIdSchema.parse(data); | ||
const { id } = result; | ||
const deletedJob = await prisma.job.delete({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets create a new field in job (deleted) which will flag it true or false instead of removing it from db
src/actions/job.action.ts
Outdated
ApproveJobSchemaType, | ||
ServerActionReturnType<ApprovedJobID> | ||
>(async (session, data) => { | ||
if (session.user.role === 'USER') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be an admin check here in case we add users with other roles
src/actions/job.action.ts
Outdated
if (session.user.role === 'USER') { | ||
throw new Error('Unauth Access'); | ||
} | ||
const result = ApproveJobSchema.parse(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use safeParse instead of parse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but other actions are using parse only, if i use safeparse there will be an other check for if(data.success)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes , it's better to safe parse
src/actions/job.action.ts
Outdated
JobQuerySchemaType, | ||
ServerActionReturnType<getAllJobsAdditonalType> | ||
>(async (session, data) => { | ||
if (session.user.role === 'USER') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refer the comment above , try makin a wrapper on top of the with session function e.g. withAdminServerAction , something like that so you wont have to repeat this step
src/actions/job.action.ts
Outdated
}); | ||
|
||
export const getAllJobsForAdmin = withSession< | ||
//not the ideal action we should make getAllJobs() better so we can have option to display verified and unverified jobs by Inputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do , add that in the getAll routes itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what im thinking is, if user role is admin return all job whether they are verified or unverified and if role is user return only verified job/ should i go with this?
<div> | ||
<Button | ||
onClick={() => { | ||
router.push('/create'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use link tag instead
|
||
const JobManagementTable = ({ jobs, searchParams, initialJobs }: props) => { | ||
const { toast } = useToast(); | ||
const [DispJobs, setDispJobs] = useState<JobType[]>(initialJobs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of just the initial jobs , can you make all of them server rendered ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was server rendered before, but because of server renders when something gets change like if job approves then the datatable was not syncing with the DB unless u hard refresh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll have to revalidate the route , and then it'll work
} | ||
}; | ||
|
||
const refreshJobs = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this function ? is it just to fetch the latest changes ?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it calls everytime searchparams changes to fetch and put latest job with local state variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need this if youre rendering table on server side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let this merge, i will update it later
const totalPages = | ||
Math.ceil((jobs.additional?.totalJobs || 0) / JOBS_PER_PAGE) || | ||
DEFAULT_PAGE; | ||
const currentPage = parseInt(searchParams.page?.toString()) || DEFAULT_PAGE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need the toString() here , if you parse the search params correctly before passing it to this table component , we wont need it
<PaginationPreviousButton | ||
searchParams={searchParams} | ||
currentPage={currentPage} | ||
baseUrl={APP_PATHS.JOBS} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also remove the need for this to be passed , its not required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pagination not required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no , we don't need the baseUrl to be paaed
@VineeTagarwaL-code done ready to merge new feat |
merge these before major updates that will be coming like user onboarding. @VineeTagarwaL-code |
@VineeTagarwaL-code solved all reviews bugs and now Table is also Server rendered. |
fix conflicts need to merge |
@VineeTagarwaL-code done |
give vid ref of current status of the pr |
finalPreview.mp4 |
A new issue will open to improve the ui of this page and possible add more functionality but great work Make sure you post your pr getting merged on x |
->Admin can visit the /manage page and see all list of active jobs, and Delete them As Well.
->From the Action button admin can edit and delete JOBS.
->Admin can Create Post. (redirects to /create Page).
->Fixed Typo in Footer
-> In this PR i have only done Delete Job Functionality which is deactivated because there is no session Management as Now.
Fixes:[#284]