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

feat: customize screenshot width for alerts/reports #24547

Merged
merged 7 commits into from
Jun 29, 2023

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Jun 28, 2023

SUMMARY

This PR is part 1 of 2. It adds a new input to the Alerts & Reports create/edit modals, as well as the report modal from charts and dashboards. In the new input users can configure a custom width for the screenshots

This PR updates the model, adds the migration to introduce a custom_width column (and custom_height, though it's not used), and add the affordances to set the custom width in the create/edit flows.

The next PR will read the information from the DB and use it when running the alerts and reports.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

New:

Screenshot 2023-06-28 at 16-45-58 FCC New Coder Survey 2018

Screenshot 2023-06-28 at 16-47-09 FCC New Coder Survey 2018

Screenshot 2023-06-28 at 16-49-27 FCC New Coder Survey 2018

TESTING INSTRUCTIONS

Create a few alerts & reports with custom width, then check the DB:

customize_screenshot_width=# SELECT id, custom_width FROM report_schedule;
 id | custom_width
----+--------------
  2 |          760
  3 |          600
  1 |          750
(3 rows)

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@betodealmeida betodealmeida requested a review from a team as a code owner June 28, 2023 23:48
@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #24547 (9ec5c8c) into master (66f59e5) will increase coverage by 0.10%.
The diff coverage is 79.71%.

❗ Current head 9ec5c8c differs from pull request most recent head 6472a95. Consider uploading reports for the commit 6472a95 to get more accurate results

@@            Coverage Diff             @@
##           master   #24547      +/-   ##
==========================================
+ Coverage   68.98%   69.08%   +0.10%     
==========================================
  Files        1906     1906              
  Lines       74114    74157      +43     
  Branches     8155     8162       +7     
==========================================
+ Hits        51124    51232     +108     
+ Misses      20871    20806      -65     
  Partials     2119     2119              
Flag Coverage Δ
hive 53.94% <67.30%> (?)
javascript 55.79% <70.58%> (+<0.01%) ⬆️
mysql 79.39% <75.00%> (?)
postgres 79.48% <75.00%> (-0.01%) ⬇️
presto 53.84% <67.30%> (+0.01%) ⬆️
python 83.47% <82.69%> (+0.20%) ⬆️
sqlite 78.04% <82.69%> (-0.01%) ⬇️
unit 54.69% <57.69%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../superset-ui-core/src/models/ExtensibleFunction.ts 100.00% <ø> (ø)
superset/reports/api.py 90.29% <ø> (ø)
superset/charts/api.py 87.78% <50.00%> (ø)
superset/reports/schemas.py 91.08% <50.00%> (-7.75%) ⬇️
...rset-frontend/src/components/ReportModal/index.tsx 77.61% <60.00%> (-1.43%) ⬇️
...-frontend/src/features/alerts/AlertReportModal.tsx 55.02% <72.72%> (+0.57%) ⬆️
...set-frontend/src/components/ReportModal/styles.tsx 100.00% <100.00%> (ø)
superset/config.py 92.04% <100.00%> (+0.06%) ⬆️
superset/initialization/__init__.py 91.04% <100.00%> (-0.31%) ⬇️
superset/reports/models.py 100.00% <100.00%> (ø)
... and 3 more

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yousoph
Copy link
Member

yousoph commented Jun 28, 2023

/testenv up FEATURE_ALERT_REPORTS = True

@github-actions
Copy link
Contributor

@yousoph Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

@yousoph Ephemeral environment creation failed. Please check the Actions logs for details.

@yousoph
Copy link
Member

yousoph commented Jun 29, 2023

/testenv up FEATURE_ALERT_REPORTS = True

@github-actions
Copy link
Contributor

@yousoph Ephemeral environment spinning up at http://54.218.34.104:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

superset-frontend/src/components/ReportModal/index.tsx Outdated Show resolved Hide resolved
superset-frontend/src/components/ReportModal/index.tsx Outdated Show resolved Hide resolved
superset-frontend/src/features/alerts/AlertReportModal.tsx Outdated Show resolved Hide resolved
superset-frontend/src/features/alerts/AlertReportModal.tsx Outdated Show resolved Hide resolved
)

@validates("custom_width")
def validate_custom_width(self, value: int) -> None: # pylint: disable=no-self-use
Copy link
Member

Choose a reason for hiding this comment

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

can we add the same validation to the UI? With some danger text under the input or sth

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, we currently show validation errors post-POST, but it would be great to show client-side.

Since this is time sensitive I'm going to do that in a separate PR, though, are you OK with that?

Copy link
Member

@hughhhh hughhhh left a comment

Choose a reason for hiding this comment

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

LGTM

@betodealmeida betodealmeida merged commit be9eb0f into master Jun 29, 2023
54 checks passed
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 8, 2024
@mistercrunch mistercrunch deleted the customize_screenshot_width branch March 26, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants