adamspd / django-appointment

A Django app for managing appointment scheduling with ease and flexibility.
Apache License 2.0
150 stars 41 forks source link

delete get_timezone and APP_TIME_ZONE #158

Closed deronnax closed 6 months ago

deronnax commented 6 months ago

get_timezone is not needed: settings.TIME_ZONE can be used and is guaranteed to exist and be valid. The fallback of APP_TIME_ZONE, despite it should never happen since TIME_ZONE necessarily exist, is misleading: nowhere in Django the default timezone is America/New York (it was the default parameter for the project generation years ago and it's not the case anymore, see the doc https://docs.djangoproject.com/en/5.0/ref/settings/#std-setting-TIME_ZONE).

Still a draft currently. Don't mind the "parallelize test runs" commit.

adamspd commented 6 months ago

The time zone function was a dirty fallback to set it to "America/New York" by default, since that's the one I needed. I'm pretty sure I put a TODO somewhere in the code to implement this the right way and remove the dirty code, but I didn't remember to do it.

I need to find a way to allow others to merge if the tests passed (I can't see if they were executed). Let me know when I can merge (once it's no longer a draft).

deronnax commented 6 months ago

I can't see if they were executed

Normally you should, once the workflow run has been granted. Btw, tests seem to not run on this MR, any idea why?

adamspd commented 6 months ago

I've been reading GitHub's documentation here and there, can't seem to find the reason why the tests are not running. I thought by adding the pull request target condition in the workflow as stated in the documentation and changing the configuration to allow them, it would work but apparently not :

name: Tests

on:
  push:
    branches-ignore:
      - 'main'
  workflow_dispatch:
  pull_request_target:
    types: [assigned, opened, synchronize, reopened]
    branches-ignore:
      - 'main'
adamspd commented 6 months ago

Still a draft currently

Still a draft ? Or can I proceed to merge it ?

deronnax commented 6 months ago

I want to review and double-check some things, I'll tell you soon, probably tonight.

adamspd commented 6 months ago

Awesome.

deronnax commented 6 months ago

And the test job keeps not running and that's really a pain, I can't work efficiently without tests runs on my PR. It seems like we are hitting a not-so-uncommon corner case of Github, there is Github discussion on the subjects here. It seems to be related to CI job name and branch protection rules of the Github repo (which I can't see), would you please give one more try? Sorry and thank you.

adamspd commented 6 months ago

Yes, I'll check it out. I agree it's annoying. I'm wondering if adding you as collaborator (if you want it too) would solve the problem or if more permission would be required.

adamspd commented 6 months ago

@deronnax I tried updating the workflow name from "tests" to "testing" and change the name in the branch rule as explained in the link in your comment and updated the settings so anyone can see the worklow and have write and read permission, but it still shows here as "Waiting for status to be reported". I tried creating a new account and added it as a collaborator, and it has the right permissions needed to see the worklow, launch them and it has the push rights directly into the repository. So, if you're interested, I can add you, but I'm gonna need the email address you used in your github account. Once added you can remove the comment if you don't want your email address to show here.

In the meantime, I saw the tests ran well in the forked repo, I can merge it and then see afterwards.

deronnax commented 6 months ago

I think you can invite me with just my handle, @deronnax . OK for being part of the repo, until we figure out this problem at least. Yes indeed tests ran well on the fork so let's merge (btw, how did you know for the fork?), but since I had some test failures locally, I wanted to be extra careful.

adamspd commented 6 months ago

I sent the invitation.

btw, how did you know for the fork?

2 reasons:

  1. GitHub has stats for the repo where you can see how many fork a repo has
  2. I participated in other repositories (golang mostly) and I had to fork to be able to send merge request.