Skip to content

Added created field to Session model - #75

Open
dwasyl wants to merge 4 commits into
jazzband:masterfrom
dwasyl:dev-created
Open

dwasyl wants to merge 4 commits into
jazzband:masterfrom
dwasyl:dev-created

Conversation

@dwasyl

@dwasyl dwasyl commented Apr 18, 2017

Copy link
Copy Markdown

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.

@Bouke Bouke left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

dwasyl commented Apr 19, 2017

Copy link
Copy Markdown
Author

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

Bouke commented Apr 20, 2017

Copy link
Copy Markdown
Collaborator

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

@dwasyl

dwasyl commented Apr 22, 2017

Copy link
Copy Markdown
Author

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

@dwasyl

dwasyl commented Sep 10, 2018

Copy link
Copy Markdown
Author

@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
Copy Markdown
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
Copy Markdown
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

blag commented May 22, 2024

Copy link
Copy Markdown
Contributor

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

@ataylor32

Copy link
Copy Markdown
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

dwasyl commented May 22, 2024

Copy link
Copy Markdown
Author

@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
Copy Markdown
Member

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

@ataylor32

Copy link
Copy Markdown
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

dwasyl commented Jun 6, 2024

Copy link
Copy Markdown
Author

@ataylor32 Sure, I should have time this weekend!

This branch has not been deployed

No deployments
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