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

create EndorsementController #41

Merged
merged 35 commits into from
Dec 31, 2023
Merged

create EndorsementController #41

merged 35 commits into from
Dec 31, 2023

Conversation

bhtibrewal
Copy link
Contributor

@bhtibrewal bhtibrewal commented Nov 23, 2023

Issue Ticket

Closes: #28

Description

  • Create postEndorsement API endpoint

Test

Screenshot 2023-11-26 at 4 26 45 PM image

Follow-up :

#80


} else {
if (endorserOptional.isEmpty())
throw new NoEntityException("User with id:" + endorserId + "not found");
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be an error as there are two throw new statements. Did you mean to throw NoEntityException for no endorsement?
Also, in the error message try to have spaces after the strings for concatenation. Otherwise for id: 123 your message will look like endorsement with id:123not found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no the endorser is checked as empty here hence user not found exception

@@ -0,0 +1,12 @@
package com.RDS.skilltree.Exceptions;

public class NoEntityException extends RuntimeException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why this is a RuntimeException and not Exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An Exception is checked, and a RuntimeException is unchecked.

Checked means that the compiler requires that you handle the exception in a catch, or declare your method as throwing it (or one of its superclasses).

Generally, throw a checked exception if the caller of the API is expected to handle the exception, and an unchecked exception if it is something the caller would not normally be able to handle, such as an error with one of the parameters, i.e. a programming mistake.

So here we are using these error to throw error for things the caller API couldn't have checked before hand like, "no user foiund for this user id " etc

Copy link
Member

Choose a reason for hiding this comment

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

@bhtibrewal and @debanjanc01 what about overwriting the stack trace in case of check and un-checked (question for both)

Copy link
Contributor

@debanjanc01 debanjanc01 Dec 5, 2023

Choose a reason for hiding this comment

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

@iamitprakash if you're talking about custom exceptions, then in my opinion it depends on how the exception is being used.

If it's Exceptions that ultimately propagates directly to the Controller and is handled by a global exception handler, then the stack trace can be suppressed. The assumption here is that the exception handler will be sending the correct response to the API caller.

If it's Exceptions that are being used internally, and handled via Catch statements, we might want to error log with the stacktrace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't the knowledge on this topic, need to read more

@@ -0,0 +1,12 @@
package com.RDS.skilltree.Exceptions;

public class NoEntityException extends RuntimeException {
Copy link
Member

Choose a reason for hiding this comment

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

@bhtibrewal and @debanjanc01 what about overwriting the stack trace in case of check and un-checked (question for both)

@@ -31,7 +31,7 @@ public class SkillModel extends TrackedProperties {
private SkillType type = SkillType.ATOMIC;

@Column(name = "is_deleted", nullable = false)
private boolean deleted;
private boolean deleted = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't boolean false by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes thanks for pointing out wasn't aware, removed it

debanjanc01
debanjanc01 previously approved these changes Dec 9, 2023
@@ -0,0 +1,4 @@
package com.RDS.skilltree.User;

public class UserController {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any use for this class?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is there in different PR, we can remove this @bhtibrewal

@heyrandhir heyrandhir mentioned this pull request Dec 14, 2023
8 tasks

// Assertions
assertNotNull(result);
assertEquals(endorserId, result.getEndorser().getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also print some message, to demonstrate what failed in the assert statements.

@vikhyat187
Copy link
Contributor

@bhtibrewal can you also write the integration tests please.

iamitprakash
iamitprakash previously approved these changes Dec 30, 2023
@iamitprakash iamitprakash self-requested a review December 30, 2023 09:23
@iamitprakash iamitprakash merged commit cfe9421 into develop Dec 31, 2023
1 check passed
@iamitprakash iamitprakash deleted the postEndorsementAPI branch December 31, 2023 13:28
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.

postEndorsement /endorsements
4 participants