Changed new business ordering to be id based#1944
Changed new business ordering to be id based#1944FarhanAliRaza wants to merge 4 commits intodjango:mainfrom
id based#1944Conversation
bmispelon
left a comment
There was a problem hiding this comment.
Hi and thanks for your contribution!
I think it would be better if we added an actual created_on field on the model and sorted by that, rather than relying on the id being sequential (which is not always guaranteed).
We'll also need a test for this new behavior.
|
I am getting this error . I am running using docker. Command is running but getting this error. |
|
@FarhanAliRaza Could you address the comments on this PR if you're still interested in working on it? Otherwise it will be closed in a week. Thanks! |
I have added |
…ved corresponding migration file.
…ield to 'created_at' in the Business model, ensuring consistent ordering of business items.
|
@bmispelon Would you like to review again? Didn't want to proceed further without your feedback. |
bmispelon
left a comment
There was a problem hiding this comment.
Hi again 😁
Thanks for adding the created_at field, I think that's a better approach than sorting on id (and that's a great use-case for auto_now_add 👍🏻 )
There's a few things I think could be improved on the PR:
-
The migration for existing data. Right now every existing
Businesswill get the current date stored in itscreated_atwhich seems less than ideal. It should at least be based on the meeting's date. And to take it even further, the previous order of items should be respected so I think the sorting should be"created_at", "title". -
Instead of testing on
response.content, I'd like to seeassertQuerySetEqualwithresponse.context["new_business"]. When doing that, you'll find out that the tests are currently broken because the value used forbusiness_typeis invalid (it should be"new"with a lowercase N). -
While we're at it I think the logic should apply to
ongoing_businesstoo, and so I would suggest changing the ordering directly on the model'sMeta.ordering.
Having said all of that, I've heard rumors that the DSF board might be moving to a new system for keeping minutes, so I'm not sure if it's worth investing a lot of time on this change. Could we get some information about this @sabderemane ?
|
Hey @bmispelon ! Yes, our Secretary confirmed and I asked for an issue on the website related to this point. Unfortunately, I missed the conversation related to this transition. Now, we need to find a way to keep providing the Having said that, I believe this PR is not needed anymore but we need to find a way to keep the meeting minutes available for people who use the RSS feed and in the website in the side bar. I'm sorry for the inconvenience @FarhanAliRaza , instead would you be interested to work on this? An issue will be created soon. |
|
@bmispelon @sabderemane As a more generalized approach, how do we feel about adding timestamp fields to each model (created_at - updated_at)? They are useful for governance and history keeping. |
Fixes #1868