Skip to content

Fixes and improvements to history views #1484

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

Merged
merged 4 commits into from
Jul 20, 2021

Conversation

ashwch
Copy link
Contributor

@ashwch ashwch commented Jul 11, 2021

  • Allow refreshing while requests are still happening by
    using a list of dict.items()
  • Fixed issue with history_refresh, it now returns SignedDataForm
    to allow Switch requests to work after refresh.
  • Clean up old rows in FE from that have now expired when refresh
    happens.
  • Fixed history_sidebar view to not raise 500 error when no toolbar
    is found for an expired store_id.
  • Old rows that have now expired, display [EXPIRED] next to the
    Switch text.

Fixes #1483

- Allow refreshing while requests are still happening by
  using a list of dict.items()
- Fixed issue with `history_refresh`, it now returns SignedDataForm
  to allow Switch requests to work after refresh.
- Clean up old rows in FE from that have now expired when refresh
  happens.
- Fixed `history_sidebar` view to not raise 500 error when no toolbar
  is found for an expired `store_id`.
- Old rows that have now expired, display [EXPIRED] next to the
  Switch text.
@codecov
Copy link

codecov bot commented Jul 12, 2021

Codecov Report

Merging #1484 (8049478) into main (961b166) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1484      +/-   ##
==========================================
- Coverage   86.43%   86.40%   -0.04%     
==========================================
  Files          35       35              
  Lines        1858     1861       +3     
  Branches      261      262       +1     
==========================================
+ Hits         1606     1608       +2     
- Misses        179      181       +2     
+ Partials       73       72       -1     
Impacted Files Coverage Δ
debug_toolbar/panels/history/views.py 87.87% <100.00%> (+1.21%) ⬆️
debug_toolbar/panels/profiling.py 86.60% <0.00%> (-2.68%) ⬇️
debug_toolbar/toolbar.py 92.04% <0.00%> (+2.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 961b166...8049478. Read the comment docs.

@matthiask matthiask requested a review from tim-schilling July 12, 2021 06:32
@matthiask
Copy link
Member

I think this looks good. I'm not sure about the change from HistoryStoreForm to SignedDataForm. Shouldn't it be possible to remove the former now?

@tim-schilling
Copy link
Member

I'm a bit swamped this week, but I will try to review this on Thu/Fri.

Copy link
Member

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, there's a lot of great stuff in here! Having the additional context of expired switches will be helpful.

I'm not sold on the need to switch away from the HistoryStoreForm though. If we don't need to use the SignedDataForm, then I'd prefer to not use that.

I'm also curious if it's possible to write a test to cover the case of parallel operations mutating the store and causing it to break. How did you encounter that case originally?

@ashwch
Copy link
Contributor Author

ashwch commented Jul 17, 2021

@tim-schilling Thanks for your review.

With the above changes the switch URLs(even after refresh) look like this:

image

If we use HistoryStoreForm, the switch buttons loaded by the initial render of History have similar signed URLs, but after hitting refresh we end up with 400 errors for any switch: http://localhost:8000/__debug__/history_sidebar/?store_id=3be1d8503a0b46999cb4cb54efc1f2a1 as this results in invalid signed_form in signed_data_view decorator: {'signed': ['This field is required.']}

Dict mutation test: It's quite tricky to prove in a test. But here's an attempt in my fork https://github.com/ashwch/django-debug-toolbar/tree/dict_mutation_test: https://github.com/ashwch/django-debug-toolbar/blob/c0454b6c20e09b0db8d1d3eae67429cd7eb07bb3/tests/panels/test_history.py#L138.

The sleep time of 0.002 could be different based on the system, hence I personally don't like the test much.

make test TEST_ARGS='tests.panels.test_history.HistoryViewsTestCase.test_parallel_history_refresh'

Copy link
Member

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

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

Thank you for the changes and further explanation. After pulling the PR and running the tests myself, it finally clicked. You're absolutely correct we should be wrapping the form should be a SignedDataForm instance, but we should still by supplying the HistoryStoreForm().initial rather than bypassing it. The purpose for this is if we ever change HistoryStoreForm's shape in the future, that will be included. Otherwise we'd have to remember to also make the change here off of pure memory which I'm probably going to fail at.

I've made the change to your branch. I'm good with these changes, thank you for the PR!

@matthiask
Copy link
Member

Thanks, both of you! I think this looks good. Let's merge this.

@matthiask matthiask merged commit 3587052 into django-commons:main Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with history view functions
3 participants