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

[Bug 766394] Auto-generate share links for KB articles #2006

Closed
wants to merge 25 commits into from

Conversation

dean
Copy link
Contributor

@dean dean commented Jun 27, 2014

This addresses bug 766394, which is linked at the bottom of this comment.

  • KB share links generated on submission of /kb/new
  • Schema migrations for a booleanfield to show the share link or not and the share link itself
  • Data migration for existing KB articles
  • Tests for the generate_short_link function

https://bugzilla.mozilla.org/show_bug.cgi?id=766394

show_share_link = forms.BooleanField(
label=_lazy(u'Share Link:'),
initial=True,
required=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably be smarter about this and automatically figure this out based on category, is_archived, etc.

@dean dean changed the title [WIP] Sharelink 766394 Sharelink 766394 Jul 3, 2014
@dean
Copy link
Contributor Author

dean commented Jul 3, 2014

Ready for review!

class Migration(DataMigration):

def forwards(self, orm):
"Write your forwards methods here."
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably edit, or at least remove this docstring ;)

@dean dean changed the title Sharelink 766394 [Bug 766394] Auto-generate share links for KB articles Jul 7, 2014
@dean
Copy link
Contributor Author

dean commented Jul 7, 2014

@mythmon Fixed the things you pointed out.

dean added 18 commits July 7, 2014 10:10
max_length is 24 characters, which should cover the url lengths
as they grow for years to come.
They are not generated yet, but there is a base set up for putting
them in the database if the box is checked.
Share links are generated via bit.lys API, and the configuration
for accessing that API can modified in settings.py.
Added migrations, form element, and now share_links are auto-generated
for each new KB article, but you can choose to hide or show them.
Also editted some styling and safeguards for KeyErrors.
We now only generate share links if the article is in a
category that should have share links generated for itself.
This is the user we should use for data migrations when having
to assign owners/creators to new objects.
This is a data migration for share links. If the template tag
sharing a link is found in the most recent revision for a document,
we then create a new revision based off the most recent one with
content that excludes the share link.
It is now done with a celery task because it's an API call and we
want it to be asynchronous. Additionally, I added more descriptive
exceptions for any status code responses form bit.ly's api other
than a 200.
We now quit the generate_short_url function if either a username
or API key is not found. There was also an update to an error
message.
dean added 5 commits July 7, 2014 10:10
There is now a cron job for generating share links where they
are missing.
We need to make sure the bitly settings are None by default, so
that if the settings are missing it is noticed by
generate_short_link in wiki/tasks.py
We now spawn up workers to take care of mass generating
share_links in chunked bits, similarly to rebuiding kb articles.
Our query for valid docs to generate share links for is now much
more efficient and should only grab articles that need the links.
Tests run and pass. Also added custom exceptions
for generate_short_url bitly errors.
This includes some comment updates and checking for bitly
exceptions instead of generic ones.
@mythmon
Copy link
Contributor

mythmon commented Jul 7, 2014

@dean you rebased, didn't you? That makes it hard to see what you changed. During reviews, you should probably avoid that.

is_template=False,
is_archived=False,
category__in=settings.IA_DEFAULT_CATEGORIES)
.exclude(slug=''))
Copy link
Contributor

Choose a reason for hiding this comment

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

How is ~Q(foo=bar) different then .exclude(foo=bar) ? Did you know you can have multiple .exclude or .filter calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried the .exclude on both of those ! statements before, without much luck, but it seems to work now. It must have been another issue. Fixed!

Removed Q filters, and subclassed BitlyException for other
bitly exceptions.
category__in=settings.IA_DEFAULT_CATEGORIES)
.exclude(slug='',
current_revision=None,
html__startswith=REDIRECT_HTML))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work as expected? I thought we had a problem where it was only filtering things that had all these properties, instead of any of these properties. If you tested it, then no worries. I was expecting something like .exclude(slug='').exclude(current_revision=None).exclude(...)

@mythmon
Copy link
Contributor

mythmon commented Jul 9, 2014

I pulled this down to test the code:

  • Ran the migrations to see if share links get moved. ✔️
  • Re-rendered the KB to make sure the old links went away. ✔️
  • Added a new document to make sure it all works without an API key. ✔️
  • Added my API key.
  • Added another document to make sure it got a share link. ✔️
  • Ran the cron job. ❌. The cron job was typoed in the crontab file.
  • Rant the correct cron job. I hit the rate limit, but I think it did the right thing. ✔️

This is almost ready. You just need to fix the cron job, and then it's ready to go.

@dean
Copy link
Contributor Author

dean commented Jul 9, 2014

Fixed the cron job typo.

@mythmon
Copy link
Contributor

mythmon commented Jul 9, 2014

Cool. r+

@dean
Copy link
Contributor Author

dean commented Jul 10, 2014

All commits squashed into these two:

Deployed to production, closing.

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.

None yet

2 participants