Skip to content

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

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

Conversation

FarhanAliRaza
Copy link

Fixes #1868

Copy link
Member

@bmispelon bmispelon left a 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.

@FarhanAliRaza
Copy link
Author

I am getting this error . I am running using docker. Command is running but getting this error.

  File "/usr/local/lib/python3.12/site-packages/asgiref/local.py", line 89, in _lock_storage
    asyncio.get_running_loop()
RuntimeError: no running event loop

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/lib/python3.12/site-packages/django/utils/connection.py", line 58, in __getitem__
    return getattr(self._connections, alias)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/asgiref/local.py", line 118, in __getattr__
    return getattr(storage, key)
           ^^^^^^^^^^^^^^^^^^^^^
AttributeError: '_thread._local' object has no attribute 'trac'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/usr/src/app/manage.py", line 22, in <module>
    main()
  File "/usr/src/app/manage.py", line 18, in main
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python3.12/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python3.12/site-packages/django/core/management/__init__.py", line 436, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python3.12/site-packages/django/core/management/commands/test.py", line 24, in run_from_argv
    super().run_from_argv(argv)
  File "/usr/local/lib/python3.12/site-packages/django/core/management/base.py", line 413, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python3.12/site-packages/django/core/management/base.py", line 459, in execute
    output = self.handle(*args, **options)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/core/management/commands/test.py", line 63, in handle
    failures = test_runner.run_tests(test_labels)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/test/runner.py", line 1072, in run_tests
    self.run_checks(databases)
  File "/usr/local/lib/python3.12/site-packages/django/test/runner.py", line 994, in run_checks
    call_command("check", verbosity=self.verbosity, databases=databases)
  File "/usr/local/lib/python3.12/site-packages/django/core/management/__init__.py", line 194, in call_command
    return command.execute(*args, **defaults)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/core/management/base.py", line 459, in execute
    output = self.handle(*args, **options)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/core/management/commands/check.py", line 81, in handle
    self.check(
  File "/usr/local/lib/python3.12/site-packages/django/core/management/base.py", line 486, in check
    all_issues = checks.run_checks(
                 ^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/core/checks/registry.py", line 88, in run_checks
    new_errors = check(app_configs=app_configs, databases=databases)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/core/checks/database.py", line 12, in check_database_backends
    conn = connections[alias]
           ~~~~~~~~~~~^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/utils/connection.py", line 61, in __getitem__
    raise self.exception_class(f"The connection '{alias}' doesn't exist.")
django.utils.connection.ConnectionDoesNotExist: The connection 'trac' doesn't exist.```

@adamzap
Copy link
Member

adamzap commented May 16, 2025

@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!

@FarhanAliRaza
Copy link
Author

FarhanAliRaza commented May 17, 2025

@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 created_on. What else is needed in this?

@FarhanAliRaza FarhanAliRaza requested a review from bmispelon May 17, 2025 17:26
@sabderemane sabderemane requested a review from a team May 19, 2025 09:25
…ield to 'created_at' in the Business model, ensuring consistent ordering of business items.
@ulgens
Copy link
Member

ulgens commented Jun 18, 2025

@bmispelon Would you like to review again? Didn't want to proceed further without your feedback.

Copy link
Member

@bmispelon bmispelon left a 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:

  1. The migration for existing data. Right now every existing Business will get the current date stored in its created_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".

  2. Instead of testing on response.content, I'd like to see assertQuerySetEqual with response.context["new_business"]. When doing that, you'll find out that the tests are currently broken because the value used for business_type is invalid (it should be "new" with a lowercase N).

  3. 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's Meta.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 ?

@sabderemane
Copy link
Member

Hey @bmispelon !
Sorry this one went off my radar.

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
meeting minutes which are now defined in a repository as markdown.

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.

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.

Board minutes Business display order should match the order they are entered in
5 participants