1843 new ajax request resets whole view if historypanel is enabled

[elineda]

Description

[+] Create a setting which enable or disable the automatic refresh when an ajax request occurs.

Fixes #1843

Checklist:

• I have added the relevant tests for this change.
• I have added an item to the Pending section of docs/changes.rst.

[matthiask]

Thank you for working on this!

[matthiask]

It's not completely consistent but we generally prefer using camelCase in JavaScript code.

Also, other parts of the code use the dataset attribute, so this should be djDebug.dataset.updateOnAjax and data-update-on-ajax (or whatever the name of the attribute is). A data- prefix for attributes is a good idea anyway because such attributes are always valid HTML.

I wonder if there is a better place for this property though. Tim suggested using localStorage to allow switching between the auto update and manual update behavior on the fly, and this probably wouldn't work with the way it's proposed here. Also, I don't think we want to expose the attribute to external modification (just yet). Putting it in the same place as the handleDragged property may be a good idea?

[tim-schilling]

Tim suggested using localStorage to allow switching between the auto update and manual update behavior on the fly, and this probably wouldn't work with the way it's proposed here.

For some context, when Elineda and I spoke yesterday, we decided we could start with a settings control. If we find that it doesn't meet developers needs, we can add the JS to make it more stateful.

[matthiask]

Makes sense!

[elineda]

I did change asked, but I don't put on a localstorage, I just with handleDragged.

[tim-schilling]

@matthiask can you review the documentation suggestions?

[matthiask]

I like it! Only small suggestions this time.

[matthiask]

Suggested change

	{{ toolbar.config.ROOT_TAG_EXTRA_ATTRS|safe }} data-update-on-fetch="{{ toolbar.config.UPDATE_ON_FETCH|safe }}">
	{{ toolbar.config.ROOT_TAG_EXTRA_ATTRS|safe }} data-update-on-fetch="{{ toolbar.config.UPDATE_ON_FETCH }}">

I'm not 100% sure but I don't think |safe is necessary.

Also, you could use |yesno:'true,false' or do it as data-default-show so that the value is lowercased already, but I'm just suggesting that, not asking for anything.

[elineda]

I removed safe. But I didn't used yesno because the result of this will be a string and we will need do to the same things in js for checking it, lowercased or not.

[tim-schilling]

@matthiask I'm realizing I broke from our typical merge process. @elineda asked in private if she could merge, I said yes.

[matthiask]

@tim-schilling No worries. I was a little bit surprised but also relieved. I think merging faster is fine, really, especially in Jazzband.

[living180]

This is a great change, thanks @elineda! I did notice that this seems to duplicate the functionality of the OBSERVE_REQUEST_CALLBACK setting, though in a much more discoverable and usable way. I did some looking at the code, and now that the UPDATE_ON_FETCH setting exists, I cannot come up with a reason for OBSERVE_REQUEST_CALLBACK to exist. If an AJAX request should not be instrumented, SHOW_REQUEST_CALLBACK can be used to block that rather than OBSERVE_REQUEST_CALLBACK (which doesn't actually block the instrumentation, but instead simply prevents an AJAX request from showing up in the history panel until the next time the history panel is loaded or refreshed). Should OBSERVE_REQUEST_CALLBACK be deprecated in favor of UPDATE_ON_FETCH?

[matthiask]

I went back and read the discussion in #1577

I think I agree with your assessment. I remember that I had some problems understanding what OBSERVE_REQUEST_CALLBACK actually meant, and having only SHOW_REQUEST_CALLBACK and UPDATE_ON_FETCH at some point in the future clarifies things a lot for me.

[living180]

OK, so I just played around with this feature some more, and either I'm doing something wrong, or it is not working the way I expect. I confirmed that I have DjDT 4.3.0 using the Versions panel, and I have an explicit UPDATE_ON_FETCH: False, in my DEBUG_TOOLBAR_CONFIG (which should be the same as the default). However, with the Debug Toolbar sidebar expanded, if I trigger an AJAX request, the sidebar still updates to show the data of that AJAX request. The behavior that I want, and was hoping for from UPDATE_ON_FETCH is that the Debug Toolbar would show the data for the request of the top-level page until I specifically select a different request from the History panel.

I am currently able to achieve that desired behavior by including 'OBSERVE_REQUEST_CALLBACK': lambda request: False, in my DEBUG_TOOLBAR_CONFIG. Is there something important I am missing?

[tim-schilling]

OBSERVE_REQUEST_CALLBACK is meant to identify which AJAX requests need to be instrumented. That setting may not be needed, but it is different from UPDATE_ON_FETCH. UPDATE_ON_FETCH is intended to determine whether or not the toolbar will automatically update with the latest AJAX request or not. As you said @living180, it should work the way you expect it to. I'll take a closer look.

[tim-schilling]

@living180 I can't reproduce the issue you're describing. When I run make example, then go to http://127.0.0.1:8000/ and click on either of the increment buttons, the toolbar doesn't update with the latest request. When I set UPDATE_ON_FETCH to False, it still doesn't update. When I set UPDATE_ON_FETCH to True, it does update. When I browse to http://127.0.0.1:8000/htmx/boost/ it works as expected as well. Would you be able to produce a reproducible example for me?

[living180]

Sure, it's late where I am, but I'll try to reproduce with a simpler example hopefully tomorrow.

[living180]

Would you be able to produce a reproducible example for me?

OK, turns out the problem on my end was caused by stale cached JavaScript files for the Debug Toolbar. After a force refresh in my browser, UPDATE_ON_FETCH does behave as expected. Apologies for the noise.

OBSERVE_REQUEST_CALLBACK is meant to identify which AJAX requests need to be instrumented.

I think SHOW_REQUEST_CALLBACK should be sufficient to identify which requests need to be instrumented, AJAX or not. Both the documentation and the code seem to indicate that OBSERVE_REQUEST_CALLBACK is simply about whether to automatically update the toolbar in response to an AJAX request or not. Even if OBSERVE_REQUEST_CALLBACK returns False, the request will still be instrumented. The only impact on behavior is whether the djdt-store-id header is included in the response: https://github.com/jazzband/django-debug-toolbar/blob/c688ce4ad7d18c5ecb800869298ca8cf6c08be1d/debug_toolbar/panels/history/panel.py#L27-L28

[tim-schilling]

OBSERVE_REQUEST_CALLBACK is used to determine if a request is being made on behalf of the toolbar or not. If we were to combine that into SHOW_REQUEST_CALLBACK, then anytime someone overrides that with return True, they'll break their integration as the toolbar will attempt to instrument it's own ajax calls. Right now my opinion is that we can't go backwards with that one. It'd break too many existing set ups and expectations.

[living180]

Unless I'm really reading something wrong, DebugToolbarMiddleware never instruments its own requests:

https://github.com/jazzband/django-debug-toolbar/blob/c688ce4ad7d18c5ecb800869298ca8cf6c08be1d/debug_toolbar/middleware.py#L48-L49

In that case the OBSERVE_REQUEST_CALLBACK callback never gets called, because it only gets called via the HistoryPanel.get_headers() method:

https://github.com/jazzband/django-debug-toolbar/blob/c688ce4ad7d18c5ecb800869298ca8cf6c08be1d/debug_toolbar/panels/history/panel.py#L23-L29

which is only called by DebugToolbarMiddleware after the early return show above:

https://github.com/jazzband/django-debug-toolbar/blob/c688ce4ad7d18c5ecb800869298ca8cf6c08be1d/debug_toolbar/middleware.py#L76-L77

So no matter how SHOW_TOOLBAR_CALLBACK or OBSERVE_REQUEST_CALLBACK are defined, the toolbar will never instrument its own requests, AJAX or otherwise.

This also means that the default definition of OBSERVE_REQUEST_CALLBACK should simply be return True because it will never be called if DebugToolbar.is_toolbar_request(request) returns False.

[tim-schilling]

Ack, good point. I wasn't digging deep enough. I agree that with the new setting, we could direct people to using SHOW_TOOLBAR_CALLBACK instead of using OBSERVE_REQUEST_CALLBACK.

[dangelsaurus]

I appreciate this enhancement, but very curious why the default is set to False when the historical behavior has been to update with an ajax request? If someone (me) misses this in the changelog, it can make for an annoying hour of troubleshooting. Forget beginner friendly, this is not a developer friendly attitude at all.

[tim-schilling]

There wasn't a great choice for us here. Your experience is the worse case for changing the default. However, that's less worse than the original bug request where the default cause hijacks the toolbar without consent.

I'm sorry you had to deal with the difficult consequences of that choice.

[tim-schilling]

The release notes could have better called it out though.

[elineda]

If I'm remember well, this pr was the reason to not put a minor version on this release.

[matthiask]

I appreciate this enhancement, but very curious why the default is set to False when the historical behavior has been to update with an ajax request? If someone (me) misses this in the changelog, it can make for an annoying hour of troubleshooting. Forget beginner friendly, this is not a developer friendly attitude at all.

Thanks for your feedback. You're very welcome to submit a concrete proposal how this could have been handled better and I'd certainly appreciate that. That being said, in my personal opinion the previous behavior wasn't friendly to the development experience of myself and others. Also, all defaults are bad for some subset of users.