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

fix: Capitalization / Sentence case under Settings #18753

Conversation

codemaster08240328
Copy link
Contributor

SUMMARY

Based on capitalization guide from superset, we need to convert Row Level Security into Row level security under settings menu.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After Updates:

image

TESTING INSTRUCTIONS

@rusackas
Copy link
Member

rusackas commented Feb 16, 2022

This is correct that according to the guidelines, Row Level Security should become Row level security - however we might as well get ALL of the items in that menu to follow the same rules. I think they should be:

  • List users
  • List roles
  • Row level security
  • Action log
  • Annotation layers
  • CSS templates (note CSS should probably be all caps)
  • Alerts & reports
  • Profile
  • Info
  • Logout

@mihir174 @kasiazjc please feel free to check my math on the above.

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

I'm a little nervous about this approach here. I don't think we need new utils to parse and change the case of items, especially if that will be implemented to handle each string differently. It seems this PR goes the route of "if it's this string, make it sentence case. If it's that string, make it title case". Rather than making string transformations based on cases, can we just find and change the actual string at the source?

@rusackas
Copy link
Member

rusackas commented Feb 16, 2022

See also this PR where the last couple of these were made uppercase to match. We should just edit those title attributes in that file for the intended purposes of this PR.

@rusackas
Copy link
Member

@dpgaspar do you recall offhand where the List Users and List Roles are set, and/or how we might override them? I'm assuming they're in a FAB Jinja template, or in a database somewhere. We want to make them sentence case (e.g. List users and List roles) per our design guidelines.

@codecov
Copy link

codecov bot commented Feb 18, 2022

Codecov Report

Merging #18753 (653787d) into master (987740a) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18753      +/-   ##
==========================================
- Coverage   66.32%   66.29%   -0.03%     
==========================================
  Files        1620     1620              
  Lines       63091    63081      -10     
  Branches     6372     6370       -2     
==========================================
- Hits        41842    41822      -20     
- Misses      19592    19602      +10     
  Partials     1657     1657              
Flag Coverage Δ
hive 52.22% <ø> (ø)
mysql ?
postgres 81.47% <ø> (ø)
presto 52.07% <ø> (ø)
python 81.86% <ø> (-0.05%) ⬇️
sqlite 81.16% <ø> (ø)

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

Impacted Files Coverage Δ
superset/initialization/__init__.py 90.65% <ø> (ø)
superset/common/utils/dataframe_utils.py 85.71% <0.00%> (-7.15%) ⬇️
superset/db_engine_specs/mysql.py 93.97% <0.00%> (-3.62%) ⬇️
...d/src/explore/components/PropertiesModal/index.tsx 68.33% <0.00%> (-1.67%) ⬇️
superset-frontend/src/views/components/Menu.tsx 55.26% <0.00%> (-1.15%) ⬇️
superset/models/core.py 88.37% <0.00%> (-0.73%) ⬇️
superset/views/core.py 77.00% <0.00%> (-0.45%) ⬇️
superset-frontend/src/views/App.tsx 0.00% <0.00%> (ø)
...s/legacy-plugin-chart-country-map/src/countries.ts 100.00% <0.00%> (ø)
...board/components/nativeFilters/FilterBar/index.tsx 69.82% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 987740a...653787d. Read the comment docs.

@dpgaspar
Copy link
Member

@dpgaspar do you recall offhand where the List Users and List Roles are set, and/or how we might override them? I'm assuming they're in a FAB Jinja template, or in a database somewhere. We want to make them sentence case (e.g. List users and List roles) per our design guidelines.

The most straightforward way, would be overriding the role model view class from FAB, but unfortunately for the user's we have multiple classes to override (basically one per auth method). So we would need to override all these: https://github.com/dpgaspar/Flask-AppBuilder/blob/master/flask_appbuilder/security/views.py#L255.

We have an ongoing PR on FAB dpgaspar/Flask-AppBuilder#1801 for adding a CRUD view around the security APIs, a better long term path would be to leverage that and build the UI around those. IMO

@rusackas
Copy link
Member

rusackas commented Mar 3, 2022

@codemaster08240328 let me know if you think the advice from Daniel above is actionable. If not, we'll hand this PR to Design and Product and see if we want to merge it (moves toward capitalization standards, but adds an inconsistency) or hold off (not to capitalization standards, but is at least consistent)

@yousoph
Copy link
Member

yousoph commented Apr 11, 2022

/testenv up

@github-actions
Copy link
Contributor

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

@rusackas
Copy link
Member

To do: Should we be concerned about localization?

@rusackas
Copy link
Member

Closing this PR for now... we can reopen later if we (perhaps with @dpgaspar's help) figure out how to make the FAB changes so that things are actually consistent stem-to-stern.

@rusackas rusackas closed this Jun 10, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

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

Successfully merging this pull request may close these issues.

None yet

4 participants