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

[3276] Fix incorrect template rendering #3279

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

msopacua
Copy link

@msopacua msopacua commented Dec 6, 2023

Description

See commit messages.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@botberry
Copy link
Member

botberry commented Dec 6, 2023

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release addresses an issue in Django's GraphQLView. Previously, variables were consumed too early when overriding the template, leading to a syntax error in the JavaScript code.

Here's the tweet text:

🆕 Release (next) is out! Thanks to msopacua for the PR 👏

Get it here 👉 https://beta.strawberry.rocks/release/(next)

Copy link

codspeed-hq bot commented Dec 6, 2023

CodSpeed Performance Report

Merging #3279 will not alter performance

Comparing msopacua:fix/3276-incorrect-template-rendering (5960f93) with main (6b84220)

Summary

✅ 11 untouched benchmarks

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Merging #3279 (5960f93) into main (6b84220) will decrease coverage by 56.09%.
The diff coverage is 100.00%.

❗ Current head 5960f93 differs from pull request most recent head 06a2632. Consider uploading reports for the commit 06a2632 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3279       +/-   ##
===========================================
- Coverage   96.59%   40.50%   -56.09%     
===========================================
  Files         482      480        -2     
  Lines       29957    29895       -62     
  Branches     3694     3685        -9     
===========================================
- Hits        28936    12110    -16826     
- Misses        835    17438    +16603     
- Partials      186      347      +161     

- Add a basic test to see if graphql template is rendering
- Add test for the failed rendering of the setting when overriding the
  template
The `get_template()` call correctly fetches the template via the engine,
but to get the same template type as the one used in the other branch,
we need to access its template attribute, where the original template is
stored.

The engine template wrap the base template to allow direct
rendering by TemplateResponse, however this conflicts with the "template
via static files" approach taken by Strawberry.

Since I'm unfamiliar with the reasoning for this approach, we'll leave
it in tact.
Test is identical, but we need the decorator on one of them.
@msopacua msopacua force-pushed the fix/3276-incorrect-template-rendering branch from 5c4686c to 54bf902 Compare December 7, 2023 13:40
Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

looks great! thanks 😊

RELEASE.md Outdated Show resolved Hide resolved
@patrick91
Copy link
Member

@msopacua looks like tests on older versions of django are failing 😊

@msopacua
Copy link
Author

Ah forgot about that. Django < 4.2 doesn't rewrite headers into WSGI environment variables yet.

Older Django versions, don't support requests-like headers for the test
client.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants