Skip to content

Feature/day 7 todos notes backend junaid#16

Open
engrjunaidali wants to merge 29 commits into
devkindhq:masterfrom
engrjunaidali:feature/day-7-todos-notes-backend-junaid
Open

Feature/day 7 todos notes backend junaid#16
engrjunaidali wants to merge 29 commits into
devkindhq:masterfrom
engrjunaidali:feature/day-7-todos-notes-backend-junaid

Conversation

@engrjunaidali

Copy link
Copy Markdown

No description provided.

Comment thread app/controllers/todos_controller.ts Outdated
Comment thread app/controllers/todos_controller.ts Outdated


async update({ params, request, response }: HttpContext) {
const todo = await Todo.findOrFail(params.id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

validation and try catch?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resolved ✅

Comment thread app/controllers/todos_controller.ts Outdated


async store({ request, response }: HttpContext) {
const data = request.only(['title', 'content', 'labels', 'imageUrl'])

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if you add validation here, then you dont have to add validation in the parse function, You are using Typescript, instead of adding type any, you can define multiple types like
labels: string | undefined | ...

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resolved ✅

Comment thread app/controllers/todos_controller.ts Outdated
private parseLabels(labels: any): string[] | null {
if (!labels) return null

if (Array.isArray(labels)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why do you need multiple types?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resolved ✅

Comment thread app/controllers/todos_controller.ts Outdated
return response.badRequest({ error: 'No image file provided' })
}

const result = await cloudinary.uploader.upload(payload.image.tmpPath!, {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

best approach is to create CloudnaryService class.
pass image path and folder name then call the upload function.

your code should look like.

await new CloudnaryService(imgPath, folder_name).upload();

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resolved ✅

Comment thread app/controllers/todos_controller.ts Outdated
folder: 'adonis_uploads',
})

return response.ok({ message: 'Image uploaded successfully', url: result.secure_url })

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What if Cloudinary throw an exception?
how you will tackle this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resolved ✅

Comment thread app/models/todo.ts
return JSON.stringify(value)
}
})
declare labels: string[] | null

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The label is a string[] or null type in the model, pass the type instead of any, and apply validation in your controller.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resolved ✅

Comment thread inertia/pages/todos/index.tsx Outdated

if (editingTodo) {
// Update existing todo
const updatedTodo: Todo = {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what about frontend validation?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resolved ✅

Comment thread inertia/pages/todos/show.tsx Outdated
export default function Show({ todo }: { todo: Todo }) {
const [isDeleting, setIsDeleting] = useState(false)

const formatDate = (dateString: string) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

move this function in the utils folder, and import wherever needed

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resolved ✅

Comment thread inertia/pages/todos/todo-card.tsx Outdated
}

export default function TodoCard({ todo, viewType, onEdit, onDelete }: TodoCardProps) {
const formatDate = (dateString: string) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

code duplication, move in the utilis

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resolved ✅

Comment thread start/routes.ts Outdated
router.get('/todos', ({ inertia }) => inertia.render('todos/empty'))
// router.get('/todos', ({ inertia }) => inertia.render('todos/empty'))

router.get('/todos', [TodosController, 'index']) // Fetch all todos

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resolved ✅

Comment thread app/services/cloudinary_service.ts Outdated
}
} catch (error) {
console.error('Cloudinary Delete Error:', error)
throw new Error('Failed to delete image from Cloudinary')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

use error.message or custom message like
error?.message ?? "Failed to delete image from Cloudinary"
it means, if error.message is undefined then return the fallback message.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resolved ✅

});


const TodoSchema = z.object({

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

use either zod or vine.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

resolved ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants