Skip to content
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

Tasking manager fastapi #6649

Open
wants to merge 157 commits into
base: develop
Choose a base branch
from
Open

Tasking manager fastapi #6649

wants to merge 157 commits into from

Conversation

prabinoid
Copy link
Contributor

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation
  • πŸ§‘β€πŸ’» Refactor
  • βœ… Test
  • πŸ€– Build or CI
  • ❓ Other (please specify)

Related Issue

Related discussions:

#6264

https://community.openstreetmap.org/t/migrating-tasking-manager-from-flask-to-fastapi-and-psycopg2-to-asyncpg/116543

Describe this PR

This PR includes the TM fastapi migration and covers the commits from the initial phases of the migration.

This PR is made for the code review of the migration and is NOT be merged currently.

kaditya97 and others added 30 commits November 27, 2024 09:56
Signed-off-by: eternaltyro <[email protected]>
Signed-off-by: eternaltyro <[email protected]>
- Build multi-stage container images on specific targets - in this case
  'prod'
- Better naming for jobs

Signed-off-by: eternaltyro <[email protected]>
- Fix outputs format
- Experiment with metadata parsing in different ways
- Add labels back to container metadata

Signed-off-by: eternaltyro <[email protected]>
Signed-off-by: eternaltyro <[email protected]>
Signed-off-by: eternaltyro <[email protected]>
Signed-off-by: eternaltyro <[email protected]>
Signed-off-by: eternaltyro <[email protected]>
prabinoid and others added 23 commits November 27, 2024 10:34
…managers assignment fixed and message module fixes
… aware

* fix: All Teams Pagination fixed
* fix: Json response for task split small area error message
* fix: Timestamps serialization in messages, comments, and notifications
* Project creation date for creation date sorting
* Email services fixed
* User statistics for validation time spent.
* Image upload in comment section.
* Last updated utc serialization in my tasks of my contributions
* Partners CRUD
* Mapswipe and statistics.
* Project partnerships.
* Request user display name made None if not authenticated.
* Routes updated for partners, partnerships and statistics.
Copy link

sonarcloud bot commented Dec 2, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
E Reliability Rating on New Code (required β‰₯ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@@ -28,7 +25,7 @@ FROM base as extract-deps
RUN pip install --no-cache-dir --upgrade pip
WORKDIR /opt/python
COPY pyproject.toml pdm.lock README.md /opt/python/
RUN pip install --no-cache-dir pdm==2.18.1
RUN pip install --no-cache-dir pdm==2.8.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small thing, but it might be worth updating pdm to 2.18.1 here to avoid downgrading on merge

router = APIRouter(
prefix="/projects",
tags=["projects"],
dependencies=[Depends(get_db)],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is needed here. Some endpoints may not need a db connection and this would create one anyway. I'm not sure how you would access the database connection variable like this anyway

"""
Get all task annotations for a project
---
tags:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entirely optional, but you could always delete this docstrings. Fastapi generates the OpenAPI docs using typing anyway, so it's a bit redundant. It also means you need to remember to update the docstring if you update variables

category_dto = await MappingIssueCategoryService.get_mapping_issue_category_as_dto(
category_id, db
)
return category_dto.model_dump(by_alias=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will work, but the preferred approach to a manual model dump is probably using response_model on the endpoint decorator. This will handle serialisation and dumping

sort_by = request.query_params.get("sortBy", "date")
sort_direction = request.query_params.get("sortDirection", "desc")
message_type = request.query_params.get("messageType", None)
from_username = request.query_params.get("from")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting approach for endpoint query params. It should work, but typically you would define query params as function arguments and they would be parsed as such. If there is confusion, it's possible to explicitly define arguments as fastapi.Query

async with db.transaction():
await CampaignService.create_campaign_organisation(
organisation_id, campaign_id, db
)
message = "campaign with id {} assigned for organisation with id {}".format(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F-strings are a bit nicer to read, but not a big deal πŸ‘

return token.model_dump(by_alias=True)


@router.patch("/authentication/applications/{application_key}/")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a PATCH method here seems a bit out of place, but maybe I am missing something

@spwoodcock
Copy link
Member

That's a lot of code changed! Well done on this - lots of great work πŸ™Œ

Just a few minor comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants