Migrate off topic channel name endpoint from the site API #26
Conversation
- Add reminder endpoint package to the project: - Add reminder dependencies - Add reminder schemas - Add reminder endpoints - Add reminder tests to the project. - The tests use mocked DB session calls, to avoid using a test DB. - Introduce changes in the flake8 config, and main to be comaptible with the test suites.
Make the test workflow compatible with the test suite using mocked DB calls
Akarys42
added
review: do not merge
type: feature
api/main.py
Outdated
| @app.exception_handler(ValueError) | ||
| async def handle_validation_value_error(req: Request, exc: ValueError) -> JSONResponse: | ||
| """A default handler to handle value errors raised by pydantic model field validators.""" | ||
| return JSONResponse(status_code=400, content={"error": f"{exc}"}) | ||
|
|
||
|
|
There was a problem hiding this comment.
I don't think that's necessary, the handler above handles every exception regarding malformed request bodies (That comes from pydantic), and returns the errors, except with the error code of 400.
There was a problem hiding this comment.
That didn't work for me 🤷🏻 though according to me it should have.
There was a problem hiding this comment.
It worked for me, and according to the documentation that is the preferred way.
(I believe the problem might have been with the 404 instead of 400.
If not, what was the test payload that failed to error out?
| def test_returns_single_item_with_random_items_param_set_to_1(self): | ||
| """Return not-used name instead used.""" | ||
| url = app.url_path_for("get_off_topic_channel_names") | ||
| response = client.get(f"{url}?random_items=1", headers=AUTH_HEADER) |
There was a problem hiding this comment.
I think you should , pass random_items using params
There was a problem hiding this comment.
It only takes URL path parameters, we are using query parameters here.
There was a problem hiding this comment.
No, the params meant to accept a dictionary of query parameters.
See, the documentation
| def get_all_otn(db_session: Session) -> Query: | ||
| """Get a partial query object with .all().""" | ||
| return db_session.query(OffTopicChannelName).all() |
There was a problem hiding this comment.
Could you move this to a separate file, into something likeotn_dependency?
There was a problem hiding this comment.
It isn't really a dependency and is only used in the current file, I don't think it deserves to go in a separate dependency file.
There was a problem hiding this comment.
If your code depends on this function, then it is a dependency, the reason I ask is because in the future new dependencies could be added, and I believe it would make the project structure more friendly.
|
|
||
| if random_items <= 0: | ||
| return JSONResponse( | ||
| status_code=404, |
There was a problem hiding this comment.
According to the API "docs":
- 400: returned when `random_items` is not a positive integer
So, this has to be 400.
|
Closing this per us sunsetting this project. |
Blocked by #25
Closes #11
I have based my branch on Dorsan's to get the test patches and routers set up. You can review this by going through the commit cfb6e87.
My commit currently has a bug in the tests where it doesn't patch
session.query.allproperly andorder_byfails to get the correct mocked object.