Skip to content

feat(HistoryRecords): prev and next attributes #365

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 19, 2018
Merged

feat(HistoryRecords): prev and next attributes #365

merged 1 commit into from
Apr 19, 2018

Conversation

@SpainTrain
Copy link
Contributor

@SpainTrain SpainTrain commented Apr 5, 2018 •
edited
Loading

TODO

  • Once API is approved, add docs on how to use this feature

Changes

  • New attributes on history model retrieve previous and next records

Closes #230

This change is Reviewable

@SpainTrain SpainTrain mentioned this pull request Apr 5, 2018
Closed
@codecov-io
Copy link

codecov-io commented Apr 5, 2018 •
edited
Loading

Codecov Report

Merging #365 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #365      +/-   ##
==========================================
+ Coverage   97.45%   97.47%   +0.02%     
==========================================
  Files          15       15              
  Lines         590      595       +5     
  Branches       73       73              
==========================================
+ Hits          575      580       +5     
  Misses          9        9              
  Partials        6        6
Impacted Files Coverage Δ
simple_history/models.py 98.51% <100%> (+0.03%) ⬆️

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 d11e393...1597816. Read the comment docs.

rossmechanic
rossmechanic reviewed Apr 6, 2018
View reviewed changes
simple_history/models.py Outdated
"""
Get the previous history record for the instance. `None` if first.
"""
entry = self.instance
Copy link

@rossmechanic rossmechanic Apr 6, 2018

Choose a reason for hiding this comment

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

I don't love going from historical record to instance back to historical record. Ideally we'd avoid this. Maybe look to see what they do in cleanerversion? https://github.com/rossmechanic/cleanerversion/blob/master/versions/models.py#L83

I think something similar to that on our historical tables would be a much cleaner solution

@SpainTrain SpainTrain force-pushed the 230-get-prev-next-record branch 3 times, most recently from 351352c to a2096b5 Compare April 13, 2018 18:04
SpainTrain
SpainTrain commented Apr 13, 2018
View reviewed changes
simple_history/models.py
"""
Get the next history record for the instance. `None` if last.
"""
return self.instance.history.filter(
Copy link
Contributor Author

@SpainTrain SpainTrain Apr 13, 2018

Choose a reason for hiding this comment

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

Good feedback. This forced me to implement these in a much more efficient and clean way. Thanks!

@SpainTrain
Copy link
Contributor Author

SpainTrain commented Apr 13, 2018

@rossmechanic should be good for another review. If reasonable, I will add relevant docs. Thanks!

@rossmechanic
Copy link

rossmechanic commented Apr 16, 2018

@SpainTrain need to pass the flake8 linting errors first.

@SpainTrain SpainTrain force-pushed the 230-get-prev-next-record branch from a2096b5 to 2c254ea Compare April 16, 2018 19:28
@SpainTrain
Copy link
Contributor Author

SpainTrain commented Apr 16, 2018

derp ✅

@SpainTrain SpainTrain force-pushed the 230-get-prev-next-record branch from 2c254ea to 9bfb2ed Compare April 16, 2018 22:27
@SpainTrain
Copy link
Contributor Author

SpainTrain commented Apr 16, 2018

docs added

@rossmechanic
Copy link

rossmechanic commented Apr 16, 2018

@SpainTrain Thanks. This looks way better to me at first glance. Going to spend some time playing around with it and get you comments by end of week.

rossmechanic
rossmechanic previously approved these changes Apr 19, 2018
View reviewed changes
Copy link

@rossmechanic rossmechanic left a comment

Choose a reason for hiding this comment

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

lgtm! Thanks!

@rossmechanic
Copy link

rossmechanic commented Apr 19, 2018 •
edited
Loading

Mind adding your name to AUTHORS.rst and the change to CHANGES.rst? Also add the PR number after the change (i.e. (gh-365)). Then I'll merge

ncvc
ncvc suggested changes Apr 19, 2018
View reviewed changes
simple_history/models.py Outdated
"""
return self.instance.history.filter(
Q(history_date__lt=self.history_date)
).order_by('history_date').first()
Copy link
Contributor

@ncvc ncvc Apr 19, 2018

Choose a reason for hiding this comment

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

This should reverse order by history date or take .last() with the existing order. We should also use say 3 records in test_get_prev_record / test_get_next_record and get the previous and next records of the last / first records, respectively, to ensure we catch this type of bug in the unit tests.

Copy link

@rossmechanic rossmechanic Apr 19, 2018

Choose a reason for hiding this comment

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

Good catch @ncvc

Copy link
Contributor Author

@SpainTrain SpainTrain Apr 19, 2018

Choose a reason for hiding this comment

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

very good catch, thanks!

@rossmechanic rossmechanic dismissed their stale review April 19, 2018 17:56

@ncvc found a bug

rossmechanic
rossmechanic suggested changes Apr 19, 2018
View reviewed changes
Copy link

@rossmechanic rossmechanic left a comment

Choose a reason for hiding this comment

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

Fix @ncvc 's bug. And edit AUTHORS.rst and CHANGES.rst

feat(HistoryRecords): prev and next attributes
1597816 
- New attributes on history model retrieve previous and next records

Closes django-commons#230
@SpainTrain SpainTrain force-pushed the 230-get-prev-next-record branch from 9bfb2ed to 1597816 Compare April 19, 2018 18:48
SpainTrain
SpainTrain commented Apr 19, 2018
View reviewed changes
Copy link
Contributor Author

@SpainTrain SpainTrain left a comment

Choose a reason for hiding this comment

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

Fix ncvc 's bug. And edit AUTHORS.rst and CHANGES.rst

✔️

simple_history/models.py Outdated
"""
return self.instance.history.filter(
Q(history_date__lt=self.history_date)
).order_by('history_date').first()
Copy link
Contributor Author

@SpainTrain SpainTrain Apr 19, 2018

Choose a reason for hiding this comment

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

very good catch, thanks!

@rossmechanic rossmechanic merged commit 2e8925b into django-commons:master Apr 19, 2018
@SpainTrain SpainTrain deleted the 230-get-prev-next-record branch April 19, 2018 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@ncvc ncvc ncvc requested changes

@rossmechanic rossmechanic rossmechanic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@SpainTrain @codecov-io @rossmechanic @ncvc