Skip to content
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

Create timeline events from Slack pins #128

Merged
merged 3 commits into from
Sep 5, 2019
Merged

Create timeline events from Slack pins #128

merged 3 commits into from
Sep 5, 2019

Conversation

milesbxf
Copy link
Contributor

@milesbxf milesbxf commented Sep 5, 2019

Adds a TimelineEvent every time a Slack pin is added and removes it when
the pin is removed.

I also decided to migrate the metadata field from
django.contrib.postgres.fields.JSONField to jsonfield.JSONField -
this supports the same features in Postgres but additionally allows the
same support in other databases - crucially, in particular, SQLite,
which means we can write tests which interact with TimelineEvents.

Adds a TimelineEvent every time a Slack pin is added and removes it when
the pin is removed.

I also decided to migrate the metadata field from
`django.contrib.postgres.fields.JSONField` to `jsonfield.JSONField` -
this supports the same features in Postgres but additionally allows the
same support in other databases - crucially, in particular, SQLite,
which means we can write tests which interact with TimelineEvents.
@milesbxf milesbxf requested a review from ChrisAnn September 5, 2019 12:43
Copy link
Contributor

@ChrisAnn ChrisAnn left a comment

Choose a reason for hiding this comment

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

Looks fine, I'm just curious about the formatting changes and quotes from single to double. Are these being done automatically?


incident = models.ForeignKey(Incident, on_delete=models.CASCADE)
timestamp = models.DateTimeField(
null=False, default=datetime.now, help_text="Time of when this event occurred."
)
text = models.TextField(help_text="Freeform text describing this event")
event_type = models.CharField(
max_length=10, choices=EVENT_TYPES, help_text="Type of event."
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we no longer want to restrict it to this enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. I realised that if people (including Monzo) want to add new custom event types to their private projects, they wouldn't be able to without forking this



class PinnedMessageManager(models.Manager):
def add_pin(self, incident, message_ts, author_id, text):
name = settings.SLACK_CLIENT.get_user_profile(author_id)['name']
name = settings.SLACK_CLIENT.get_user_profile(author_id)["name"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen single quotes elsewhere. Not that it's a blocker, I would like to know what's Python idiomatic :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah - unfortunately Python allows both, and both styles are widely used! Even the "official" Python style guide PEP-8 doesn't make a recommendation.

I use black as a formatter locally - we should probably standardise on this

@danpalmer
Copy link
Contributor

@milesbxf for my understanding, why would we want a timeline event to be removed? I can imagine pinned messages changing over time and that we wouldn't leave a message pinned after it is no longer relevant, but might want to see that on the timeline? Or is there another intended use-case here?

@milesbxf
Copy link
Contributor Author

milesbxf commented Sep 5, 2019

@milesbxf for my understanding, why would we want a timeline event to be removed? I can imagine pinned messages changing over time and that we wouldn't leave a message pinned after it is no longer relevant, but might want to see that on the timeline? Or is there another intended use-case here?

@danpalmer good question. Internally we've found quite often in an incident we'll pin loads of messages as we go along to help us pick out info later. When we're compiling the report we often remove a lot of pinned messages, as they aren't generally written in a way that is easy to read (missing context) and add a lot of noise

@danpalmer
Copy link
Contributor

@milesbxf Cool thanks for the explanation!

@milesbxf milesbxf merged commit a418faf into master Sep 5, 2019
@milesbxf milesbxf deleted the slack-pins branch September 5, 2019 14:16
@milesbxf milesbxf mentioned this pull request Sep 5, 2019
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.

3 participants