Notifications Improvements

[ttannis]

Summary of Changes

This PR contains a set of improvements for the notifications work.

1. Instead of passing the resource_url using window.location.href to get the url of the resource to send along in the email, the option has been renamed resource_path. The code that sends the notification is responsible for generating the resource_path -- e.g. /table_detail/.../.../.... The logic in python that generates the email content will create the final url by concatenating the app.config['FRONTEND_BASE'] with the resource_path to generate the full url.

   • This change was made because we wanted to have explicit control over the generation of the url.
   • In order to achieve this we now pass the full tableMetadata to RequestMetadataForm and generateOwnerUpdateRequest so those entities can generate the resource_path.

2. The piece of application state -- notification that controls the state of the RequestMetadataForm has two new optional parameters: requestMetadataType and columnName. requestMetadataType allows us to auto-select "Table Description" or "Column Description" when opening the form. columnName is passed so that we can include it in the default comment when requesting column descriptions.
   

3. In addition to including a default comment if the user opened RequestMetadataForm from the column description, the comment becomes required. If the default comment is deleted we prompt the user to enter which columns they would like better descriptions for.

4. Changing the recipients input from type="email" to type="text" honors the spacing between emails that we want. It seems on type="email" the space is stripped, and type="text" currently works for our use case as we aren't leveraging any of the extra features available with type="email".

5. Updated RequestMetadataForm styles to make the text in the textarea darker.

Tests

Modified unit tests according to code changes.

Documentation

N/A. No new functionality or features have been added that need to be documented.

CheckList

Make sure you have checked all steps below to ensure a timely review.

• PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.
• PR includes a summary of changes, including screenshots of any UI changes.
• PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public python functions and the classes in the PR contain docstrings that explain what it does
• PR passes all tests documented in the developer guide

[ttannis]

Will cleanup this branch then change PR to merge into master

[ttannis]

Not sure how this got closed.

[ttannis]

@danwom already merged amundsen-notifications down to master and tried to clean this one up. Commit history is all over the place but the current diff is at least accurate.

Still need changes reviewed before finishing the unit tests.

[danwom]

I wonder if we can share code with getLink in TableListItem to generate links.

[ttannis]

Given the discrepancies between the schema we get from the search service and the schema we get from the metadata service, I opted not to write that method since it would have more branching logic than common logic accounting for the differences in schemas + logging params.
Yet another reason to unify the schemas sooner rather than later to make the code on the frontend more maintainable.

[danwom]

I see.

If I could redo this logic, the URL would just be table_details?key=${table key} and then we'd never need to deal with the whole match params and lengthy url building.

[ttannis]

We can still do this if the team agree and we ever have opportunity to prioritize it or do some tech debt work. We would just have to write some kind of migration support code so that old url patterns convert to new url patterns.

[danwom]

Instead of raising an exception if FRONTEND_BASE has a / and when resource_path doesn't have a /, could we do a smart join to quietly handle either case?

[ttannis]

We could but then the question is how. Only thing I can think of to resolve your concern without turning this into url validation project is to join the two and check the two characters at the intersection (the character that exists at the length of FRONTEND_BASE, and the character directly after) and check that one and only one of those characters is a /?

[ttannis]

Implemented the approach I mentioned above.

[danwom]

Still not a big fan of 'columnIndex', especially considering that sorting and filtering columns are coming up. We could probably pass an entire TableColumn or even just the name.

[ttannis]

I can go with the name. The thinking here was to avoid passing a repetitive chunks of data.