-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feature/362 categories #378
Conversation
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.
Awesome job, congratz on your first PR this semester 🎉! Well done, I can really see you understand the underlying architecture of the application.
Your pull request did not have a description and steps to reproduce. I understand the general idea of this pull request, but it would be nice if you could update the pull request anyways (it's easier for others to review).
Your code looked really good. I found some small notes and made some suggestions, check if you agree with me (if you don't agree, let me know why 😉). Keep up the good work Jurjen!
Co-authored-by: Ruben Fricke <55654104+Ruby77@users.noreply.github.com>
Co-authored-by: Ruben Fricke <55654104+Ruby77@users.noreply.github.com>
Co-authored-by: Ruben Fricke <55654104+Ruby77@users.noreply.github.com>
What should be considered as steps to reproduce in this case? Reproduce what? Expected functionality? |
Good question @JVerbruggen! In Steps to Test or Reproduce you explain how to test your code. So in your case, this could be something like this: "Execute the CRUD functionality from the tag controller as admin and validate that other roles are not able to do this. Validate you are able to add, update and delete tags on your own project. Validate that the data officer is able to update/add/delete tags on projects from users from the same institute. And finally, validate that the admin is able to update/add/delete tags on projects from everyone.". I hope this makes sense, let me know otherwise 😉 |
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.
Nicely done! I have a few requests. Good job that you also updated Postman tests and unit tests. 🥇
We will meet about the system tomorrow to discuss if the implementation matches how we envision it.
|
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.
Good work! 👍
I tried to update the categories of an existing project but something went wrong mapping the resource to an actual project. Maybe just to be sure you can also add a check for this in the integration tests.
Also found an issue for the authorization check.
Maybe since you have to make some changes you can also fix Tim's feedback.
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.
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.
Thanks for your implementation :) Good work! 💯
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.
Everything works flawless. Thanks for the pull request!
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 👍
Categories
Description
The addition of tags can give users a better insight into certain characteristics of projects. This pull request introduces tags to dex.
Type of change
Checklist
Steps to Test or Reproduce
Outline the steps to test or reproduce the PR here.
These steps will be used during release testing.
Link to issue
Closes: #362