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

Added created field to Session model #75

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dwasyl
Copy link

@dwasyl dwasyl commented Apr 18, 2017

In response to Issue #66 I added the field myself and created this PR. Shouldn't negatively impact anything, and I made the migration set the created to be equal to the value of last_activity rather than being ahead of it for backward compatibility.

Copy link
Collaborator

@Bouke Bouke left a comment

Choose a reason for hiding this comment

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

Could you have a look at the failing tests and my comment?

field=models.DateTimeField(default=datetime.datetime(2017, 4, 18, 12, 11, 0, 286186), auto_now_add=True),
preserve_default=False,
),
migrations.RunPython(set_created),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Migrations should be reversible, so please also provide the reverse_code argument here. It doesn't need to do anything; see also this snippet from the Django docs:

Pass the RunPython.noop method to code or reverse_code when you want the operation not to do anything in the given direction. This is especially useful in making the operation reversible.

@dwasyl
Copy link
Author

dwasyl commented Apr 19, 2017

Fixed the data migration reversing, but the TravisCI failures are going to take some extra work on my part. Best I can tell they are related to https://code.djangoproject.com/ticket/17654 and the way the db.py save function updates existing records. That said, in production I'm not running into any errors.

@Bouke
Copy link
Collaborator

Bouke commented Apr 20, 2017

Maybe we should use force_insert in the tests when creating the sessions? Does that resolve the issue? See also our db backend.

@dwasyl
Copy link
Author

dwasyl commented Apr 22, 2017

That might help, I'm going to do some testing over the weekend.

@dwasyl
Copy link
Author

dwasyl commented Sep 10, 2018

@bourke, I did some testing at the time and have been using my own fork for a while, but figure it's time to get this PR finalized. The code still works fine, just the tests fail due to auto_add_now an re-using of the pk. Any ideas to get around this by changing the tests?

Alternatively, could use a datetimefield and override save to set the value manually, but that's not ideal.

@ataylor32
Copy link
Contributor

What's the status of this pull request? I would really like to have this feature in a project I'm working on.

@WhyNotHugo
Copy link
Member

This project was unmaintained for some years and this PR went stale during that period.

Currently, it has several merge conflicts, so would need to be rebased.

@blag
Copy link
Contributor

blag commented May 22, 2024

Check out django-qsessions for a drop-in replacement that supports a created_at field.

@ataylor32
Copy link
Contributor

@blag Thanks for the suggestion. I may end up doing that, but it might be difficult because my project is already using django-user-sessions.

@WhyNotHugo If @dwasyl updates this pull request (or if they're not able or willing to, I could create a new pull request), would merging it relatively soon be a possibility? What is the release process? I see that it has been close to one and a half years since the last release was made. My project would need this in the next couple weeks and I'm wondering if that's realistic or if I should look at other options.

@dwasyl
Copy link
Author

dwasyl commented May 22, 2024

@ataylor32 glad it's not just me that likes the functionality. I've used it in production for a while without any issues, the only problem I ever had was in running the test suite due to the auto_add_now and the re_use of pk's. There never seemed to be a preference or interest in it so I left it as is and used it.

Let me see if I have an updated fork to push.

@WhyNotHugo
Copy link
Member

Yeah, a rebased version of this sounds good to merge.

@ataylor32
Copy link
Contributor

@dwasyl Would you be able to update this pull request anytime soon? If not, I can open a separate pull request to replace this one.

@dwasyl
Copy link
Author

dwasyl commented Jun 6, 2024

@ataylor32 Sure, I should have time this weekend!

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.

5 participants