-
Notifications
You must be signed in to change notification settings - Fork 3
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(Teacher): Implement unit tagging #276
Conversation
feat(Tag): Implement create tag endpoint
feat(Tag): Use tag objects instead of strings in project
feat(Tag): Implement delete tag endpoint
…e-tags feat(Tag): Prevent creating a duplicate tag
…-tags feat(Tag): Prevent duplicate tags when editing tag
feat(Tag): Implement tag color
…ects fix(Unit Library): Fix tags returned in projects
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.
The functionality mostly works.
It looks like the number of tags in the message when you try to delete a tag is off. In the screenshot below, you can see that I have "tag3" on a unit, but the cofirmation message says "0" I think it may not be counting the tags that are on the units (as opposed to run units).
I also added some code cleaning suggestions inline.
Lastly, maybe we should also apply the code cleaning conventions for at least the new standalone components, like alphabetizing (imports -> selector -> standalone...), rename "styleUrls" -> "styleUrl", self-closing tags, and such?
Oops. The comment above was intended for the WISE-Client PR. I have yet to review this PR. |
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.
Does this PR need to make changes to wise_db_init.sql?
Also, see inline comments. I wonder if the functions in [UserTag,TagProject]Controller should return the object directly instead of wrapping it in ResponseEntity
. I think Spring automatically returns 200 (unless there's an error), and 404 (if the function exists due to an exception)?
src/main/java/org/wise/portal/presentation/web/controllers/tag/TagProjectController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/wise/portal/presentation/web/controllers/tag/TagProjectController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/wise/portal/presentation/web/controllers/tag/UserTagController.java
Outdated
Show resolved
Hide resolved
I removed the ResponseEntity from TagProjectController.java. I kept it in the UserTagController.java because the client side looks for the error field to contain 'tagAlreadyExists' to show an error message. I've also updated wise_db_init.sql to include the color column in the user_tags table. |
For error handling- can we do something similar to how we handle invalid name when creating user accounts? This will separate out the concerns and make the functions in UserTagController easier to read. They won't need to wrap the return value in ResponseEntityGenerator.createSuccess/createError each time. |
Ok, we now throw a TagAlreadyExistsException. |
See my inline comment in UserTagController. Do we need to use ResponseEntityGenerator? |
Ok, I've removed the usage of ResponseEntityGenerator. |
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.
LGTM
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.
Looks good. 👍
🎉 This issue has been resolved in version 1.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Changes
Run this mysql database query to add the color column to the user_tags table
Test
Closes #259