Skip to content

[tap-frontapp] - fix libraries and dependabot issues #27

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

romilqlik
Copy link

Description of change

Updated FrontApp tap with Schema fixes and unit tests.
Fixed and streamlined 'init.py by ensuring all streams and their fields have proper metadata with 'selected: true' at both stream and field levels (breadcrum[]).
Added .circleci/config.yml to trigger CircleCI pipeline

Manual QA steps

Ran Discovery
Ran full sync
Validated against FrontApp API docs.
Unit test executed

Risks

None

Rollback steps

  • revert this branch

AI generated code

https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code

  • this PR has been written with the help of GitHub Copilot or another generative AI tool

setup.py Outdated
@@ -25,4 +25,4 @@
"schemas": ["tap_frontapp/schemas/*.json"]
},
include_package_data=True
)
)

Choose a reason for hiding this comment

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

Add new line at end of the file.

Choose a reason for hiding this comment

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

I see new line is still missing.

# Stream-level metadata select the stream
metadata.append({
"metadata": {
"selected": True # Make sure to select every stream

Choose a reason for hiding this comment

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

@gl-romil selected is a non-discoverable metadata which can't written or read by the tap. Please refer getting-started documentation. Also I find some required discoverable metadata is missing from the catalog, refer docs for more details.

Copy link
Author

Choose a reason for hiding this comment

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

Replaced 'selected' with discoverable metadata fields.
Added missing 'inclusion', inclusion-reason' , and 'selected-by-default'.

# Creating a dict to change before converting
catalog_dict = catalog.to_dict()

required_streams = [

Choose a reason for hiding this comment

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

You can use streams.METRIC_API_DESCRIPTION_KEY instead of introducing new variable which already defines these stream names to required.

Copy link
Author

Choose a reason for hiding this comment

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

Replaced static stream list with streams.METRIC_API_DESCRIPTION_KEY.keys()

Copy link

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

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

Requested changes in discover() which may need changes in the other tap implementation as well as in the tests. Please take of those as well.

@romilqlik romilqlik force-pushed the feature/SAC-27536-frontapp-libraries branch from 976e331 to 9f6165a Compare May 20, 2025 10:39
Copy link
Author

@romilqlik romilqlik left a comment

Choose a reason for hiding this comment

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

All review comments are addressed.

# Stream-level metadata select the stream
metadata.append({
"metadata": {
"selected": True # Make sure to select every stream
Copy link
Author

Choose a reason for hiding this comment

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

Replaced 'selected' with discoverable metadata fields.
Added missing 'inclusion', inclusion-reason' , and 'selected-by-default'.

# Creating a dict to change before converting
catalog_dict = catalog.to_dict()

required_streams = [
Copy link
Author

Choose a reason for hiding this comment

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

Replaced static stream list with streams.METRIC_API_DESCRIPTION_KEY.keys()

Copy link
Author

@romilqlik romilqlik left a comment

Choose a reason for hiding this comment

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

Kindly review

@romilqlik romilqlik force-pushed the feature/SAC-27536-frontapp-libraries branch from 2b2a85e to 9f6165a Compare May 21, 2025 10:13
* [SAC-27536] Refactored discovery and sync- logic

* [SAC-27535] Final clean up: discovery fixes,init and sync

* Update discover.py

Removed type annotations from the file as per review

* Update schemas.py

Removed type annotations from file as per review

* Update sync.py

Removed type annotations as per review

* Update sync.py

* Update __init__.py

Removed type annotations as per comment

* Update __init__.py

* Update __init__.py

Removed unnecessary header as per review

* Update __init__.py

* Update __init__.py

Removed header comment

* Update sync.py

Removed file level docstring

* Update test_client.py

Add EOF

---------

Co-authored-by: gl-romil <[email protected]>
def discover():
"""Run the discovery mode, prepare the catalog file and return the catalog."""
schemas, field_metadata = get_schemas()
print("Schemas loaded:", schemas.keys())

Choose a reason for hiding this comment

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

Please replace print() but LOGGER.info()

Choose a reason for hiding this comment

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

Also we need to do credential validation as we discussed for tap-braintree.

Copy link
Author

Choose a reason for hiding this comment

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

This is Done, Kindly check

romilqlik added 2 commits May 29, 2025 17:01
Added LOGGER.info
Credentials Validation
Copy link
Author

@romilqlik romilqlik left a comment

Choose a reason for hiding this comment

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

Created validate_credential function in init.py file
Replaces print with LOGGER

Comment on lines 34 to 46
def validate_credentials(token):
"""Validates the FrontApp token using a simple API call"""
headers = {"Authorization": f"Bearer {token}"}
try:
response = requests.get("https://api2.frontapp.com/me", headers=headers, timeout=10)
if response.status_code == 200:
LOGGER.info("Frontapp credentials validated successfully.")
else:
LOGGER.critical("Invalid Frontapp credentials. Status code: %s", response.status_code)
sys.exit(1)
except requests.exceptions.RequestException as err:
LOGGER.critical("Credential validation failed: %s", str(err))
sys.exit(1)

Choose a reason for hiding this comment

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

Move this to discovery.py

Copy link
Author

Choose a reason for hiding this comment

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

This is Done, Kindly review

Copy link
Author

@romilqlik romilqlik left a comment

Choose a reason for hiding this comment

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

Kindly review

Comment on lines 79 to 81

if __name__ == "__main__":
unittest.main()

Choose a reason for hiding this comment

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

Please remove this but not holding PR for this.

Copy link
Author

Choose a reason for hiding this comment

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

I have removed it.

@RushiT0122 RushiT0122 self-requested a review May 30, 2025 11:50
Copy link
Author

@romilqlik romilqlik left a comment

Choose a reason for hiding this comment

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

Removed
if name == "main":

  • unittest.main()

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

Successfully merging this pull request may close these issues.

3 participants