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

dalai2547/sucu-52-documents-update-doc-by-id #11

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

D33102
Copy link

@D33102 D33102 commented Sep 29, 2024

No description provided.

@D33102 D33102 self-assigned this Sep 29, 2024
@D33102 D33102 changed the title feat: document update and delete dalai2547/sucu-52-documents-update-doc-by-id Sep 29, 2024
@D33102
Copy link
Author

D33102 commented Sep 29, 2024

Screenshot 2567-09-29 at 12 40 41 Screenshot 2567-09-29 at 00 35 07

Comment on lines 80 to 88
existingDocument, err := u.documentRepository.FindDocumentByID(ID)
if err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
u.logger.Named("UpdateDocumentByID").Error(constant.ErrDocumentNotFound, zap.String("documentID", ID))
return apperror.InternalServerError(constant.ErrDocumentNotFound)
}
u.logger.Named("UpdateDocumentByID").Error(constant.ErrFindDocumentByID, zap.String("documentID", ID), zap.Error(err))
return apperror.InternalServerError(constant.ErrFindDocumentByID)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure about this, but I think u.documentRepository.UpdateDocumentByID will return gorm.ErrRecordNotFound if there's no documents with that id. So, you don't have to u.documentRepository.FindDocumentByID and maybe move if errors.Is(err, gorm.ErrRecordNotFound) into if err := u.documentRepository.UpdateDocumentByID(ID, updateMap)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure too bro @bookpanda, please check it kub @D33102

Copy link
Author

@D33102 D33102 Sep 30, 2024

Choose a reason for hiding this comment

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

I don't think it will return ErrRecordNotFound since when I tested with non-recorded ID, it didn't give any error. @bookpanda @wiraphatys

Copy link
Author

@D33102 D33102 Sep 30, 2024

Choose a reason for hiding this comment

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

Screenshot 2567-09-30 at 15 03 48

Copy link
Author

Choose a reason for hiding this comment

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

Should I do this instead?
Screenshot 2567-09-30 at 15 06 36

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks that looks good to me

Comment on lines 100 to 109
existingDocument, err := u.documentRepository.FindDocumentByID(ID)

if err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
u.logger.Named("DeleteDocumentByID").Error(constant.ErrDocumentNotFound, zap.String("documentID", ID))
return apperror.InternalServerError(constant.ErrDocumentNotFound)
}
u.logger.Named("DeleteDocumentByID").Error(constant.ErrFindDocumentByID, zap.String("documentID", ID), zap.Error(err))
return apperror.InternalServerError(constant.ErrFindDocumentByID)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

also here

Comment on lines 124 to 132
err := h.documentUsecase.UpdateDocumentByID(documentID, updateDocumentDTO)
if err != nil {
if err.Error() == "document not found" {
resp := response.NewResponseFactory(response.ERROR, "Document not found")
return resp.SendResponse(c, fiber.StatusNotFound)
}
resp := response.NewResponseFactory(response.ERROR, "Failed to update document")
return resp.SendResponse(c, fiber.StatusInternalServerError)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use this instead so that the logic for checking status code is all in usecase layer

apperr := h.documentUsecase.UpdateDocumentByID(documentID, updateDocumentDTO)
	if apperr != nil {
		resp := response.NewResponseFactory(response.ERROR, apperr.Error())
		return resp.SendResponse(c, apperr.HttpCode)
	}

Comment on lines 155 to 163
err := h.documentUsecase.DeleteDocumentByID(documentID)
if err != nil {
if err.Error() == constant.ErrDocumentNotFound {
resp := response.NewResponseFactory(response.ERROR, constant.ErrDocumentNotFound)
return resp.SendResponse(c, fiber.StatusNotFound)
}
resp := response.NewResponseFactory(response.ERROR, constant.ErrDeleteDocumentFailed)
return resp.SendResponse(c, fiber.StatusInternalServerError)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

also here

userRouter.Post("/", httpHandler.Middleware().IsLogin, httpHandler.Middleware().SuperAdmin, httpHandler.User().CreateUser)
userRouter.Patch("/", httpHandler.Middleware().IsLogin, httpHandler.User().UpdateProfile)
userRouter.Put("/:user_id", httpHandler.Middleware().IsLogin, httpHandler.Middleware().SuperAdmin, httpHandler.User().UpdateUserByID)
userRouter.Post("/", httpHandler.User().CreateUser)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you updated with dev branch?

Copy link
Contributor

Choose a reason for hiding this comment

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

please update with latest dev branch kub

Comment on lines 134 to 135
documentsRouter.Post("/", httpHandler.Document().CreateDocument)
documentsRouter.Delete("/:document_id", httpHandler.Document().DeleteDocumentByID)
Copy link
Contributor

Choose a reason for hiding this comment

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

For post/delete routes, let's add some middlewares to e.g.

documentsRouter.Post("/", httpHandler.Middleware().IsLogin, httpHandler.Middleware().SuperAdmin, httpHandler.Document().CreateDocument)
documentsRouter.Delete("/:document_id", httpHandler.Middleware().IsLogin, httpHandler.Middleware().SuperAdmin, httpHandler.Document().DeleteDocumentByID)

If you have any problems trying to test routes that requires login, we can talk na

userRouter.Post("/", httpHandler.Middleware().IsLogin, httpHandler.Middleware().SuperAdmin, httpHandler.User().CreateUser)
userRouter.Patch("/", httpHandler.Middleware().IsLogin, httpHandler.User().UpdateProfile)
userRouter.Put("/:user_id", httpHandler.Middleware().IsLogin, httpHandler.Middleware().SuperAdmin, httpHandler.User().UpdateUserByID)
userRouter.Post("/", httpHandler.User().CreateUser)
Copy link
Contributor

Choose a reason for hiding this comment

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

please update with latest dev branch kub

}

func (s *FiberHttpServer) initAttachmentRouter(router fiber.Router, httpHandler handlers.Handler) {
attachmentRouter := router.Group("/attachments")

attachmentRouter.Post("/:document_id", httpHandler.Attachment().CreateAttachments)
attachmentRouter.Get("/:document_id", httpHandler.Attachment().CreateAttachments)
Copy link
Contributor

Choose a reason for hiding this comment

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

please update with latest dev branch kub

Comment on lines 134 to 135
documentsRouter.Post("/", httpHandler.Document().CreateDocument)
documentsRouter.Delete("/:document_id", httpHandler.Document().DeleteDocumentByID)
Copy link
Contributor

Choose a reason for hiding this comment

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

apply middleware here kub, both using .IsLogin() middleware kub

@D33102
Copy link
Author

D33102 commented Sep 29, 2024

@bookpanda @wiraphatys
Update:
Screenshot 2567-09-29 at 23 36 46
Uploading Screenshot 2567-09-29 at 23.36.51.png…

Delete:
Screenshot 2567-09-29 at 23 25 46
Screenshot 2567-09-29 at 23 25 57

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.

3 participants