-
Notifications
You must be signed in to change notification settings - Fork 15
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
923 add categories for software #984
Conversation
f4470d5
to
6451bd0
Compare
478bb62
to
4dd933c
Compare
@dmijatovic it seems that only you can switch off draft state ;) Please also review this PR (can not add you) |
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.
Nice work! I have a few small comments/questions:
- What is
frontend/.yarn/releases/yarn-1.22.19.cjs
? It is 5MB in size. Can it be removed? Same forfrontend/.yarnrc.yml
. - Please remove the
AND is_published
from the policy 'anyone_can_read' oncategory_for_software
, that logic is already covered by the policy on software. - Please capitalise 'as' everywhere in
category_paths_by_software_expanded
- In the test, please put the
User
class in a dedicated file. - I'm not sure about the
User
constructor using HTTP requests to initialise itself. I think it's better to have a static factory method somewhere that does this for you and returns aUser
upon success. But this could be done later as well. - Line 273 can be deleted as it only contains a unnecessary semicolon.
- Maybe a test can be added that tries to create a cycle in the categories.
- Is it intentional that there is not an admin interface to add/edit categories?
Works well, some comments:
|
4dd933c
to
ce6b54a
Compare
Thank you for your feedback! :)
We have already seen that generic categories inspire lots of ideas what we could do next :) |
But that is already covered by A small pet peeve of mine, the example SQL file is indented with spaces instead of tabs. :) That's all from me, nice 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.
ce6b54a
to
5ac0b9b
Compare
@ewan-escience understood now the hidden magic :) -> fixed Also finally fixed Sonarcube code smells. I do not agree with one rule considering |
[rsd-database] Kudos, SonarCloud Quality Gate passed!
|
[rsd-frontend] Kudos, SonarCloud Quality Gate passed! |
PR #946 was polished and prepared for v2 -> created this new PR.
How to test:
remove extra file extension from
999-add-categories.sql.example
, optional edit file and finally build application as usual.On a software edit page you will find a new headline "Categories".