Support: update contact information via Front webhook #8406

stsewd · 2021-08-12T19:49:57Z

Added a new endpoint to listed to front webhooks, the endpoint validates the signature using the api secret from Front.

I have already created the rules and webhooks (they need to be marked as active and the urls updated after deploy), so we only need to set the secrets on the ops repos.

stsewd · 2021-08-12T22:42:06Z

readthedocs/urls.py

@@ -34,6 +34,7 @@
    url(r'^support/error/$',


I can move these to the new support module in this PR if that's ok.

Seems reasonable 👍

ericholscher

This feels pretty specific to our hosted RTD instances. I think this probably belongs in -ext, but I don't feel too strongly. Seems we made the API client generic, but didn't make the actual integration generic. It seems like a weird middle step. I think it's fine for now, but if we actually want pluggable things, we probably want a Front client for syncing user data, not for the API itself, and an API that's like backend = settings.DATA_BACKEND(); backend.sync_data(custom_data) or similar.

ericholscher · 2021-08-13T20:11:05Z

readthedocs/support/front.py

+import requests
+
+
+class FrontClient:


This seems like overkill for 3 API calls. Is there a reason that we're using a wrapper instead of just calling the API directly?

we need to pass the token header on each request, felt like a lot of duplication, I'm fine moving this class to the views file. Can also merge this into the view class, but not a big fan of fat-views.

ericholscher · 2021-08-13T20:13:15Z

readthedocs/urls.py

@@ -34,6 +34,7 @@
    url(r'^support/error/$',


Seems reasonable 👍

humitos

I left some suggestions about:

not use 500 errors when it's not our error
improve log messages by adding the URL requested to it
rename "front" to be "frontapp"" when possible to make it clear that we refer to the Front Support service

humitos · 2021-08-16T08:24:55Z

readthedocs/support/views.py

+    Currently we only listen to inbound messages events.
+    Contact information is updated when a new message is received.
+
+    See https://dev.frontapp.com/docs/webhooks-1.


Suggested change

See https://dev.frontapp.com/docs/webhooks-1.

See https://dev.frontapp.com/docs/webhooks-1

Make the link clickable 😄

Works for me with the period (gnome-terminal and kitty)

He, it doesn't work on GitHub 🙃

readthedocs/support/urls.py

readthedocs/support/views.py

humitos · 2021-08-16T08:28:18Z

readthedocs/support/views.py

+        except Exception:  # noqa
+            msg = f'Error while getting conversation {conversation_id}'
+            log.exception(msg)
+            return Response({'detail': msg}, status=HTTP_500_INTERNAL_SERVER_ERROR)


I don't think we should return 500 here. It's not our own error. Maybe 400 or 404 is better.

This is an error when connecting to the front API, not an error on how the request was done, so not a 4xx.

readthedocs/support/views.py

humitos · 2021-08-16T08:30:59Z

readthedocs/support/views.py

+            msg = f'Error while getting contact {email}'
+            log.exception(msg)
+            return Response({'detail': msg}, HTTP_500_INTERNAL_SERVER_ERROR)


Same here. 500 is not a good error IMO for external errors.

Also, the exception message could be improved by showing the URL tried: url=...

humitos · 2021-08-16T08:36:10Z

This feels pretty specific to our hosted RTD instances. I think this probably belongs in -ext, but I don't feel too strongly.

It makes sense to me putting this in the -ext repository, 👍🏼

Seems we made the API client generic, but didn't make the actual integration generic. It seems like a weird middle step. I think it's fine for now, but if we actually want pluggable things, we probably want a Front client for syncing user data, not for the API itself, and an API that's like backend = settings.DATA_BACKEND(); backend.sync_data(custom_data) or similar.

This sounds complex and extra work for us IMO, and I'm not sure that somebody will use it.

stsewd · 2021-08-16T15:37:42Z

I think this probably belongs in -ext, but I don't feel too strongly.

yeah, I put it here since the support form was already here. I could move everything in another PR.

ericholscher · 2021-08-16T15:44:13Z

The support form being here makes sense, it's core logic. The integration with Front though is very custom, so almost all OSS users won't care about it, so it doesn't really make sense in the .org codebase.

stsewd · 2021-08-16T15:48:56Z

rename "front" to be "frontapp"

+1 on that, actually while searching for an existing integration with "django + front" results were mostly about frontend...

stsewd · 2021-08-16T16:23:34Z

Should I move this to -ext now or merge this PR first?

humitos · 2021-08-17T10:31:20Z

Should I move this to -ext now or merge this PR first?

Probably better to move it now to keep things clean.

stsewd force-pushed the update-profile-info-front branch 5 times, most recently from f253670 to cf001a6 Compare August 12, 2021 22:20

Support: update contact information via Front webhook

bb29a38

stsewd force-pushed the update-profile-info-front branch from cf001a6 to bb29a38 Compare August 12, 2021 22:20

stsewd marked this pull request as ready for review August 12, 2021 22:34

stsewd commented Aug 12, 2021

View reviewed changes

stsewd requested review from a team and removed request for a team August 12, 2021 22:50

ericholscher approved these changes Aug 13, 2021

View reviewed changes

humitos reviewed Aug 16, 2021

View reviewed changes

Updates from review

8305c6d

stsewd force-pushed the update-profile-info-front branch from 0e3ff61 to 8305c6d Compare August 16, 2021 16:20

Move things to readthedocs-ext

144187b

humitos approved these changes Aug 19, 2021

View reviewed changes

stsewd merged commit 6d3cb4b into master Aug 24, 2021

stsewd deleted the update-profile-info-front branch August 24, 2021 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support: update contact information via Front webhook #8406

Support: update contact information via Front webhook #8406

stsewd commented Aug 12, 2021 •

edited

Loading

stsewd Aug 12, 2021

ericholscher Aug 13, 2021

ericholscher left a comment

ericholscher Aug 13, 2021

stsewd Aug 16, 2021

ericholscher Aug 13, 2021

humitos left a comment

humitos Aug 16, 2021

stsewd Aug 16, 2021

humitos Aug 17, 2021

humitos Aug 16, 2021

stsewd Aug 16, 2021

humitos Aug 16, 2021

humitos commented Aug 16, 2021

stsewd commented Aug 16, 2021

ericholscher commented Aug 16, 2021

stsewd commented Aug 16, 2021

stsewd commented Aug 16, 2021

humitos commented Aug 17, 2021

	See https://dev.frontapp.com/docs/webhooks-1.
	See https://dev.frontapp.com/docs/webhooks-1

Support: update contact information via Front webhook #8406

Support: update contact information via Front webhook #8406

Conversation

stsewd commented Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Aug 16, 2021

stsewd commented Aug 16, 2021

ericholscher commented Aug 16, 2021

stsewd commented Aug 16, 2021

stsewd commented Aug 16, 2021

humitos commented Aug 17, 2021

stsewd commented Aug 12, 2021 •

edited

Loading