Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

Fixes bug 1069039 - Adjust start times for event assignments #100

Closed
wants to merge 7 commits into from

Conversation

bugzPDX
Copy link
Contributor

@bugzPDX bugzPDX commented Oct 17, 2014

Start times for Event Assignment iCal feeds adjusted to 30
minutes prior to event start time in order to allow staff
sufficient set-up time for events.

Start times for Event Assignment iCal feeds adjusted to 30
minutes prior to event start time in order to allow staff
sufficient set-up time for events.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d331d1e on bugzPDX:1069039_bug into 1973a09 on mozilla:master.

@peterbe
Copy link
Contributor

peterbe commented Oct 17, 2014

What's missing is a test that proves this.
If you're unsure, where to start writing a test, I suggest this:

  1. Insert a raise Exception('here!') near the line you just added.
  2. Then run the whole test suite: ./manage.py test
  3. Note in the errors you get which test(s) cover this code.
  4. Open those files and either amend an existing test or copy the test and write a new one that just checks that the time returned is different from what's entered on the event.

@@ -873,7 +873,8 @@ def events_calendar_ical(request, privacy=None):
for event in events:
vevent = cal.add('vevent')
vevent.add('summary').value = event.title
vevent.add('dtstart').value = event.start_time
vevent.add('dtstart').value = (event.start_time -
datetime.timedelta(minutes=30))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the correct change. However, let's get picky...

Can you change to use this style:

vevent.add('dtstart').value = (
    event.start_time - datetime.timedelta(minutes=30)
)

that way you don't have that massive long thing of just empty space that depends on the length of "vevent.add('dtstart').value".
This is important because right now the line "[ SPACE ]datetime.timedelta(minutes=30))" depends on the length of the line above. So, if we some day change the word "dtstart" to "dtend" or we change the variable "vevent" to "ve" then the indentation breaks down and becomes out of alignment which means you need to edit other lines too which makes the diff change things that don't actually are changing.

Secondly, this is a peculiar little hack. 6 months from now, someone might wonder. Why this?! And why 30min? Why not 45min?!
So, I think you should add a verbose (1-2 lines) comment explaining this. Try to avoid the exact number "30 minutes" in the comment.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d435eb0 on bugzPDX:1069039_bug into 1973a09 on mozilla:master.

vevent.add('dtend').value = (event.start_time +
datetime.timedelta(hours=1))
# Adjusted start times for Event Assignment iCal feeds
# to allow staff sufficient time for event set-up.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent!

@peterbe
Copy link
Contributor

peterbe commented Oct 20, 2014

So I have a rule about the code style. The rule is that consistency is more important than minutiae. All python code (except some rare exceptions) need to pass pyflake and pep8. The way I enforce this is by having a script check all .py code before I git commit. To set it up for yourself, follow these steps:
https://github.com/mozilla/airmozilla#pep8-and-pyflakes

@peterbe
Copy link
Contributor

peterbe commented Oct 20, 2014

The errors aren't your fault. I'm going to merge your patch manually. It's looking good now.

@peterbe
Copy link
Contributor

peterbe commented Oct 20, 2014

Merged d664a36

@peterbe peterbe closed this Oct 20, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants