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.

Sign up for GitHub

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

Jump to bottom

Avoid "File name too long" error #1128

Merged
merged 12 commits into from
Dec 30, 2024
Merged

Avoid "File name too long" error #1128

merged 12 commits into from
Dec 30, 2024

Conversation

araglu
Copy link
Contributor

@araglu araglu commented Dec 5, 2024 •
edited
Loading

Description

This merge request addresses #1127 where CSS text greater than 256 characters causes an OSError exception on many pages, including admin pages.

Changes Made to Code:

  • On any file error, treat "var" as CSS text to embed
  • Add docstring
  • Reorder imports
  • Remove unnecessary else: after return
  • Add unit test

Related

Additional Notes

  • While making the unit test, I found that added a /* CSS Comment */ bypassed the issue. So the test variable is a little shorter than I hoped for. Also, that test CSS is not going to win any design awards.

Quality Checks

  • New code is 100% tested
  • Code has been formatted (black)
  • Code has been linted (pylint, flake8)
  • Docstrings for new methods have been added

@coveralls
Copy link

coveralls commented Dec 5, 2024 •
edited
Loading

Coverage Status

coverage: 99.973% (-0.03%) from 100.0%
when pulling 5cb1046 on araglu:long-css
into 70ef31f on tethysplatform:main.

swainn
swainn reviewed Dec 6, 2024
View reviewed changes
Copy link
Member

@swainn swainn left a comment

Choose a reason for hiding this comment

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

Thanks for the patch @araglu. See my comment.

tethys_apps/templatetags/site_settings.py Outdated Show resolved Hide resolved
@araglu
Log a warning if var looks like a filename but can't load
921c6da 
The login page calls load_custom_css() twice with an empty var, so this now returns early for that.
@araglu
Update code style test
ca7a2d1 
load_custom_css() now returns an empty string for a file that does not exist, so test_get_css_is_code() has been updated to test CSS code.
@araglu
Apply black formatting
9dd9c58
swainn
swainn requested changes Dec 30, 2024
View reviewed changes
Copy link
Member

@swainn swainn left a comment

Choose a reason for hiding this comment

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

A few suggestions for comments to aid future devs. Feel free to refine/correct.

tethys_apps/templatetags/site_settings.py Outdated Show resolved Hide resolved
tethys_apps/templatetags/site_settings.py Outdated Show resolved Hide resolved
tethys_apps/templatetags/site_settings.py Show resolved Hide resolved
tethys_apps/templatetags/site_settings.py Show resolved Hide resolved
@araglu
Copy link
Contributor Author

araglu commented Dec 30, 2024

I like all those changes. Thanks!

@swainn swainn merged commit e95effb into tethysplatform:main Dec 30, 2024
42 checks passed
@swainn
Copy link
Member

swainn commented Dec 30, 2024

All merged. Thank you for the contribution @araglu!

@araglu araglu deleted the long-css branch December 31, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swainn swainn swainn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@araglu @coveralls @swainn