Feature/basic api #1
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bart-maykin
force-pushed
the
feature/basic-api
branch
9 times, most recently
from
ee1eef3 to
02f978e
Compare
bart-maykin
force-pushed
the
feature/basic-api
branch
from
02f978e to
39f0b47
Compare
annashamray
requested changes
May 3, 2024
There was a problem hiding this comment.
It's a nice simple API, so le'ts make code clean and simple
- Please make CI green and happy
- Please remove jenkins from the project if you are not going to use it (including all mentioning in the docs)
- README.rst needs some love: description, keywords and correct links are nice to have
- INSTALL.rst also needs some love
- link to repo are not corrected
- fixtures loaded in Docker section don't exist
- "building and publishing the image" section uses file which doesn't exist
- Please remove all unused code
- 'ci' folder. Or is it used somewhere?
- commands in 'bin/' folder, for example all 'celery_*' ones
- "tests/test_celery_beat" doesn't seem to be needed
- "templates/sniplates", since you don't have sniplates in the reqs
- "templates/hijack", since you don't have hijack in the reqs
- "templates/samples" or is it used?
- "utils/pdf"
- Admin dashboard doesn't look nice, all the resources are put in the one "overige" group. It can be addressed in the next PR, in this case please create an issue for it
- Helptext icons in the admin forms are displayed strangely
| # | ||
| # You may wish to alter this file to override the set of languages analyzed, | ||
| # or to provide custom queries or build logic. | ||
| name: "CodeQL" |
There was a problem hiding this comment.
I'm not sure private repositories can have code scanning
bart-maykin
force-pushed
the
feature/basic-api
branch
from
ea9eeb4 to
f452792
Compare
SonnyBA
reviewed
May 7, 2024
bart-maykin
force-pushed
the
feature/basic-api
branch
from
b07cce5 to
f642335
Compare
bart-maykin
force-pushed
the
feature/basic-api
branch
from
f642335 to
7bab4d5
Compare
bart-maykin
force-pushed
the
feature/basic-api
branch
from
feb291e to
2d0c1af
Compare
annashamray
requested changes
May 10, 2024
There was a problem hiding this comment.
It looks like these things were not addressed:
- Admin dashboard doesn't look nice, all the resources are put in the one "overige" group. > It can be addressed in the next PR, in this case please create an issue for it
- Helptext icons in the admin forms are displayed strangely
| # | ||
| # You may wish to alter this file to override the set of languages analyzed, | ||
| # or to provide custom queries or build logic. | ||
| name: "CodeQL" |
bart-maykin
force-pushed
the
feature/basic-api
branch
from
26a2088 to
d3a0872
Compare
|
@annashamray can you please elaborate what you mean with "Helptext icons in the admin forms are displayed strangely"? |
|
@bart-maykin this is how Item admin page looks for me Help-text icons should be near the field names, not below |
annashamray
approved these changes
May 14, 2024
| @@ -1,18 +1,20 @@ | |||
| import os | |||
| from pathlib import Path | |||
There was a problem hiding this comment.
Pathlib is considered more modern than os.path, so I think replacing it back was unnecessary
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #5
Added basic api with token auth, redoc and landing page