[GitHub] [superset] villebro commented on pull request #17429: fix: allow POST chart/data request without CSRF token

[GitBox <gi...@apache.org>]

villebro commented on pull request #17429:
URL: https://github.com/apache/superset/pull/17429#issuecomment-968712833


   Thanks for the fix @etr2460 . We've been seeing similar regressions in other PRs lately, many of which I've unfortunately been party to, either as an author or reviewer. I think it's important for everyone to accept that the state of test coverage is what it is, and we need to do our best to
   1) make sure we keep Superset as functional as possible
   2) do our best to encourage adding more tests to existing critical functionality going foward
   
   I agree with @ofekisr that it's a tall order to expect every developer to have full understanding of what side-effects a code change can have. If this is an implicit requirement (=having full understanding of what breakage may occur despite CI being green), then it will become increasingly difficult for new community contributions to get through the review pipeline.
   
   Regarding this regression, optimally #10397 that originally introduced the functionality would have added an integration test that made sure the endpoint works without CSRF tokens (in hindsight, as a reviewer, I should have pushed for that). But in the meantime, whenever we do refactors to code that may be dangerous, it's probably a good idea to request reviews from additional people who may have more context, especially with a very detailed PR description and targeted questions (e.g. "are there any known consequences of moving the x endpoint from palce A to B?") to make it easier for reviewers to jump in and not have to spend considerable time parsing the intent of the PR.
   
   Having said that, I'm happy to start coordinating an effort to add test coverage to code that has either 1) been subject to a regression 2) is known to have a high risk of regressions due to lacking test coverage.
   
   Ping @john-bodley @junlincc 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org