implement uploads for sponsor logos in admin #9792
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
berinhard
reviewed
Jul 14, 2021
Comment on lines
+34
to
+39
| warnings.warn( | ||
| "LocalSponsorLogoStorage is intended only for use in development, you " | ||
| "should not use it in production due to the lack of safe guards " | ||
| "for safely locating files on disk.", | ||
| InsecureStorageWarning, | ||
| ) |
berinhard
reviewed
Jul 14, 2021
| fp.write(request.POST["color_logo"].file.read()) | ||
| extension = os.path.splitext(request.POST["white_logo"].filename)[-1] | ||
| filename = f"{sponsor.id}-color-logo{extension}" | ||
| form.color_logo_url.data = storage.store(filename, fp.name) |
There was a problem hiding this comment.
This is great! By updating color/white_logo_url we won't have to change anything were the logo is being displayed 👍
di
reviewed
Jul 14, 2021
| request.session.flash("Sponsor updated", queue="success") | ||
| return HTTPSeeOther(location=request.current_route_path()) | ||
| if request.method == "POST": | ||
| _handle_images(request, form) |
There was a problem hiding this comment.
Maybe this should only be called if the form is valid?
| redirect_url = request.route_url("admin.sponsor.list") | ||
| return HTTPSeeOther(location=redirect_url) | ||
| if request.method == "POST": | ||
| _handle_images(request, form) |
There was a problem hiding this comment.
form won't validate if the white_logo is being uploaded and we don't have a url yet :/
other option is to fingerprint each upload, which is fine, I think I'll will address in #9794 also.
There was a problem hiding this comment.
(note fingerprinting will effectively mean that we'll "collect" a bunch of old logos in the bucket over time... which is probably OK?)
Comment on lines
+107
to
+120
| if request.POST.get("white_logo") not in [None, b""]: | ||
| with tempfile.NamedTemporaryFile() as fp: | ||
| fp.write(request.POST["white_logo"].file.read()) | ||
| storage = request.find_service(ISponsorLogoStorage) | ||
| extension = os.path.splitext(request.POST["white_logo"].filename)[-1] | ||
| filename = f"{sponsor_name}-white-logo{extension}" | ||
| form.white_logo_url.data = storage.store(filename, fp.name) | ||
| if request.POST.get("color_logo") not in [None, b""]: | ||
| with tempfile.NamedTemporaryFile() as fp: | ||
| storage = request.find_service(ISponsorLogoStorage) | ||
| fp.write(request.POST["color_logo"].file.read()) | ||
| extension = os.path.splitext(request.POST["color_logo"].filename)[-1] | ||
| filename = f"{sponsor_name}-color-logo{extension}" | ||
| form.color_logo_url.data = storage.store(filename, fp.name) |
There was a problem hiding this comment.
Seems like we could benefit from some de-duplication here. Maybe storage.store needs an interface for storing a data blob instead of a filename?
di
added a commit
that referenced
this pull request
Jan 4, 2022
* chore: replace deprecated scalar method SQLAlchemy 1.4.x deprecated the `.as_scalar()` method and replaced it with `.scalar_subquery()` and issued a `SADeprecationWarning`. Refs: https://docs.sqlalchemy.org/en/14/changelog/changelog_14.html#change-f413ff8c13a1fb7d766bfa16dfcdfbf1 Signed-off-by: Mike Fiedler <miketheman@gmail.com> * test: ignore InsecureStorageWarning When another class was added, it wasn't excluded from the warnings report. Refs: #9792 Original ignore: #4360 Signed-off-by: Mike Fiedler <miketheman@gmail.com> * test: ignore CIText warnings in test output The updated SQLAlchemy library now emits 2,000+ warnings for CIText(). I've opened an issue on the responsible repo. Refs: mahmoudimus/sqlalchemy-citext#25 Signed-off-by: Mike Fiedler <miketheman@gmail.com> * test: filter out known legacy behavior in test The test case knowingly creates invalid versions, and the code handles them by performing some extra validation, and throws the error. Suppress the warnings for creating `LegacyVersion`. Signed-off-by: Mike Fiedler <miketheman@gmail.com> * test: filter out SAWarning for invalidation The collections used in the test case are intentionally being modified. Signed-off-by: Mike Fiedler <miketheman@gmail.com> Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
domdfcoding
pushed a commit
to domdfcoding/warehouse
that referenced
this pull request
Jun 7, 2022
* implement uploads for sponsor logos in admin * correct the interface * fix _most_ of the tests * process sponsor image uploads in create and edit views * repair all existing tests * linting * test new services
domdfcoding
pushed a commit
to domdfcoding/warehouse
that referenced
this pull request
Jun 7, 2022
* chore: replace deprecated scalar method SQLAlchemy 1.4.x deprecated the `.as_scalar()` method and replaced it with `.scalar_subquery()` and issued a `SADeprecationWarning`. Refs: https://docs.sqlalchemy.org/en/14/changelog/changelog_14.html#change-f413ff8c13a1fb7d766bfa16dfcdfbf1 Signed-off-by: Mike Fiedler <miketheman@gmail.com> * test: ignore InsecureStorageWarning When another class was added, it wasn't excluded from the warnings report. Refs: pypi#9792 Original ignore: pypi#4360 Signed-off-by: Mike Fiedler <miketheman@gmail.com> * test: ignore CIText warnings in test output The updated SQLAlchemy library now emits 2,000+ warnings for CIText(). I've opened an issue on the responsible repo. Refs: mahmoudimus/sqlalchemy-citext#25 Signed-off-by: Mike Fiedler <miketheman@gmail.com> * test: filter out known legacy behavior in test The test case knowingly creates invalid versions, and the code handles them by performing some extra validation, and throws the error. Suppress the warnings for creating `LegacyVersion`. Signed-off-by: Mike Fiedler <miketheman@gmail.com> * test: filter out SAWarning for invalidation The collections used in the test case are intentionally being modified. Signed-off-by: Mike Fiedler <miketheman@gmail.com> Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.