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

Check missing links for engage pages #87

Merged
merged 1 commit into from
Nov 25, 2021

Conversation

carkod
Copy link
Contributor

@carkod carkod commented Mar 8, 2021

Done

  • Check missing links for engage pages

QA

Use this PR

Issue

Fixes #75

@nottrobin
Copy link
Contributor

As you suggested face-to-face, let's make use of flask.current_app.extensions["sentry"] to notify Sentry of these problems - probably you want captureMessage.

As I mentioned you should be able to locally test sending sentry messages by setting the SENTRY_DSN env var in .env.local. I think.

@nottrobin nottrobin changed the title Check missing links for engage pages WIP: Check missing links for engage pages Mar 15, 2021
@carkod
Copy link
Contributor Author

carkod commented Mar 15, 2021

As you suggested face-to-face, let's make use of flask.current_app.extensions["sentry"] to notify Sentry of these problems - probably you want captureMessage.

As I mentioned you should be able to locally test sending sentry messages by setting the SENTRY_DSN env var in .env.local. I think.

You also suggested to directly put the error messages on /engage, and I thought that was a better solution, that will allow not only developers but the marketing team to be able to spot these issues, as they will not likely check Sentry. Do I still need to add it to Sentry? The error messages will be the same.

@nottrobin
Copy link
Contributor

@carkod I thought we agreed on doing both.

Do errors on the engage page even catch every problem? Aren't there going to be 500 pages if you try to visit the engage pages in question? Or does something else happen?

Anyway, we should do both.

@carkod carkod changed the title WIP: Check missing links for engage pages Check missing links for engage pages Mar 19, 2021
@carkod
Copy link
Contributor Author

carkod commented Mar 19, 2021

@carkod I thought we agreed on doing both.

Do errors on the engage page even catch every problem? Aren't there going to be 500 pages if you try to visit the engage pages in question? Or does something else happen?

Anyway, we should do both.

So Sentry has to be attached to a service, so it doesn't work on the module, I have done it on ubuntu.com PR you should see a message like this once you add the DSN of that sentry project to your env.local

@SirSamTumless
Copy link

What's happening with this @carkod ?

@nottrobin
Copy link
Contributor

nottrobin commented Mar 30, 2021

@carkod presumably you could simply pass through the SENTRY_DSN to this module (make it optional) and then this module could do the reporting? 'cos everywhere would benefit from this functionality.

@nottrobin
Copy link
Contributor

@carkod really? that doesn't work? What happened when you tried?

@SirSamTumless
Copy link

@carkod What's happening with this - will you tackle it this week?

@carkod carkod force-pushed the engage-check-missing-link branch 2 times, most recently from 953bcc9 to f35e277 Compare June 8, 2021 10:27
Copy link
Contributor

@nottrobin nottrobin left a comment

Choose a reason for hiding this comment

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

Tried switching to 18033 as you suggested and going to /engage - got:

image

Copy link
Contributor

@nottrobin nottrobin left a comment

Choose a reason for hiding this comment

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

Once the two comments are addressed (and you make the linter happy), I'm happy with this. So once it's unblocked by canonical/ubuntu.com#9601, we can do a final test and merge it.

@carkod carkod force-pushed the engage-check-missing-link branch 2 times, most recently from cf99d3b to b609278 Compare November 24, 2021 17:00
@carkod carkod force-pushed the engage-check-missing-link branch from b609278 to 58bb463 Compare November 24, 2021 17:09
@carkod carkod merged commit 02f26a5 into canonical:main Nov 25, 2021
@carkod carkod deleted the engage-check-missing-link branch November 25, 2021 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle no topic link safely
3 participants