Closed laghee closed 6 years ago
Looks really good! I'll let @karlcow have a peek too.
@karlcow Thanks for the resources. And this:
It's often easier to have data-in -> (massaging) -> data-out. Then write.
...makes tons of sense.
I'm going to try extracting the database actions into discrete db-utility functions (assuming that doesn't cause issues with session/context -- probably flask_sqlalchemy can handle it):
issue
, event
, label
, milestone
operations
oradd_thing()
, remove_thing()
, update_thing()
(or magic-idea-that-occurs-while-tinkering)
UPDATE: Experimenting with the same webhooks set up on my old local webcompat test-repo, it turns out that GH does some weird stuff with issue events. When milestones/labels are added while opening an issue, they still send out separate webhooks for those "additional" events.
Example:
- I opened an issue with a milestone and two labels.
- Not only did the milestone/labels come through in the original
opened
action,- That also triggered one
milestoned
action and two labeled
actions (which... fine, I guess?)- But then the truly bizarre thing is that each of the two separate labeled
webhook payloads contains both labels. 🤔 😕
So... odd and dumb that we could potentially perform busywork data updates that aren't actually changing anything. But probably won't break too much.
This means I need to add some logic for processing multiple labels, though. I had assumed there would only ever be one label per payload. (ahhh, assumptions... <insert mildly offensive cliche here>
)
EDIT: Errrrhm, never mind! Turns out I mixed up the labels
key in the payload with the label
key. (Note to self: Never try to parse anything too intricate on a Friday afternoon. 🤦♀️ )
@miketaylr @karlcow I made changes based on discussion and requests:
dict.get
usages to dict['key']
header
usages to title
to match github (also updated in models.py PR #52)make_change_to_database
)update_db
) back into the webhook handler to make testing easier (hopefully)Going back to hacking on tests with my new resources...
Tests are failing for this (tiny) linting error: