Status goals #56
Status goals #56
Conversation
| @@ -152,7 +154,6 @@ public void editGoal(@PathParam("id") Long goalId, GoalFormDto newGoalDto) { | |||
| } | |||
|
|
|||
| @POST | |||
| @RolesAllowed({"ADMIN", "USER"}) | |||
There was a problem hiding this comment.
Why was this removed? Do you want to allow users without these roles to use these methods?
There was a problem hiding this comment.
they were moved to the top of the GoalApi because that all the methods are allowed to all users in GoalApi .
There was a problem hiding this comment.
OK. Did not see the class-level annotation at first, due to GitHub collapsing unchanged code.
| Optional<Goal> goal = goalStore.getGoalById(goalId); | ||
| if (goal.isPresent()) { | ||
| Goal g = goal.get(); | ||
| if (g.getUser() == user) { |
There was a problem hiding this comment.
Only primitives, Strings and Enums may be compared this way. Never use == to compare objects unless you are explicitly comparing references to the same object. Use equals() (making sure the class implements both equals() and hashCode()), or compare user IDs.
There was a problem hiding this comment.
It may be possible that they are the same object due to entity persistence framework (Hibernate), however we cannot rely on that.
There was a problem hiding this comment.
added equals() and hashCode()
| private List<Goal> CheckStatus(List<Goal> goals) { | ||
| List<Goal> goalsToReturn = new ArrayList<>(); | ||
| for (Goal goal : goals) { | ||
| if ((goal.getDeadlineDate().isBefore(LocalDate.now()) |
There was a problem hiding this comment.
Please extract LocalDate.now() to a variable and place it outside the for loop, to ensure consistent comparison of all goals against a single date and to reduce the amount of calls to LocalDate.now().
There was a problem hiding this comment.
extracted thank you.
src/main/webapp/app/js/goal-page.js
Outdated
| if (response.status === 204) { | ||
| onLoad(); | ||
| } else { | ||
| console.log(response); |
There was a problem hiding this comment.
Might be worth it to extract console.log() and alert() to a single function which reports the error (e.g. displayError(response), and reuse that function over the code.
There was a problem hiding this comment.
alert() extracted to function displayErrors() and removed all console.log()
| return false; | ||
| } | ||
| displayError(response, 200); | ||
| return response.json(); |
There was a problem hiding this comment.
Will this return work as intended in case of error?
There was a problem hiding this comment.
Yes, displayError() does redirect in case of error so the rest of the code will not start.
src/main/webapp/app/js/redirects.js
Outdated
| @@ -6,10 +6,10 @@ function logout() { | |||
| }); | |||
| } | |||
| function redirectToGoalsAndComments(id) { | |||
| if (id >= 0) { | |||
| location.href = path + "/app/goal.jsp?id=" + id; | |||
| if (id === "-9999") { | |||
There was a problem hiding this comment.
Reason is on the main page if you do not have a any goals it will create a fake entry which on-click redirects to add page using this id.
src/main/webapp/app/js/redirects.js
Outdated
| } | ||
|
|
||
| function displayError(response, expected) { | ||
| if (response.status === expected) { |
There was a problem hiding this comment.
The if condition should be inverted to avoid an empty block, i.e.:
if (response.status !== expected) {
...
}
src/main/webapp/app/js/redirects.js
Outdated
| if (response.status === expected) { | ||
| } else { | ||
| alert("Something went wrong! error: " + response.status.toString()); | ||
| history.go(-1); |
There was a problem hiding this comment.
What is the reason for automatically going back to the previous page in this case?
There was a problem hiding this comment.
There is no reason to stay on page where there is no data so you will be redirected to your last page. The question is should i redirect to some error page or not? because there is a warning with error already.
Checklist
mvn testfrom the root directory to see all new and existing tests passMotivation and Context
#52
Description
this functionality is required for tracking goals statuses.