-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[FEATURE] Added ability for data docs links in emails #10408
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
base: develop
Are you sure you want to change the base?
[FEATURE] Added ability for data docs links in emails #10408
Conversation
|
Name | Link |
---|---|
🔨 Latest commit | bb4b2d3 |
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: pascal06s |
@cla-bot check |
|
||
def _build_report_element_block(self, validation_result_urls: list[str]) -> str | None: | ||
for url in validation_result_urls: | ||
fixed_url = pathlib.Path(urllib.parse.unquote(url)).as_posix() |
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.
I'm confused by this step. It looks like the net result is going from file:///myfile
to file:/myfile
. My understanding is that the former is correct. Why not just pass through the url that the user configured?
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.
You're right, the first part is unnecessary! Unquoting, however, is needed to parse the urls that are supplied in format ... /gx/uncommitted/data_docs/local_site/validations%5Ccompanies%5Ctestsuite%5C ...
…_expectations into feature/email_docs_link
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.
I want to run through this manually today. Once I've done that and CI is passing, this looks great! LMK if you need a hand with getting CI passing
tests/render/test_EmailRenderer.py
Outdated
html_blocks = email_renderer.render( | ||
validation_result=validation_result, | ||
data_docs_pages=data_docs_pages, | ||
validation_result_urls=["file://localsite/index.html"], |
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.
Non-blocking, but it may be worth updating this to a url-encoded string so that we can demonstrate the unquote
call
I noticed that the TeamsRenderer now seems a bit more up-to-date than the SlackRenderer that this code is based on. I'll change the overall handling when i am back next week. |
✅ Deploy Preview for niobium-lead-7998 canceled.
|
Added ability for
EmailRenderer
to render data docs links in emails similar to howSlackRenderer
handles it. (#10324)invoke lint
(usesruff format
+ruff check
)For more information about contributing, see Contribute.
After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!