-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Changed new business ordering to be id
based
#1944
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Business
will get the current date stored in itscreated_at
which 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 seeassertQuerySetEqual
withresponse.context["new_business"]
. When doing that, you'll find out that the tests are currently broken because the value used forbusiness_type
is invalid (it should be"new"
with a lowercase N). -
While we're at it I think the logic should apply to
ongoing_business
too, 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. |
Fixes #1868