[chore] Fixes #42 - Import old wordpress data and convert to markdown… #98

benichu · 2015-12-03T10:54:51Z

Left TODOS:

document execution of rake command on heroku
cleanup NewsItem model
controller tests for news_items#show and news_items#index
unit tests .published

PROBLEMS:

Overriding thoughtbot/administrate does not work as advertised

benichu · 2015-12-03T11:02:41Z

waiting for #83 to be merged before finishing it up

houndci-bot · 2015-12-05T01:39:54Z

config/routes.rb

@@ -11,7 +11,11 @@
  devise_for :users

  resources :events, only: [:index, :show]
+
+  # NewsItem compatibility with old wordpress Posts url
+  get "/:year/:month/:slug", to: "news_items#show", constraints: { year: /\d{4}/, month: /\d{2}/ }


Line is too long. [98/80]

houndci-bot · 2015-12-05T17:04:19Z

app/models/news_item.rb

+# NOTE: [legacy]
+# - This is also where we imported the old wordpress posts
+# - The `slug` is purely used to store the old wordpress slug and make sure we
+#   can still resolve: http://www.montrealrb.com/[post_date:YYYY]/[post_date:MM]/[post_name]


Line is too long. [92/80]

benichu · 2015-12-05T17:08:00Z

@montrealrb/organizers If anyone of you is interested to do a code-review and merge that PR, it takes care of the following:

Generate a wordpress database dump in db/legacy.yml
Create a rake task to import that legacy data in the news_items table
Add some unit and controller specs for NewsItem
Deal with legacy urls: /YYYY/mm/a-wordpress-slug

Note: you will noticed that I imported all the posts from wordpress (hopefully) but that I only marked the 10 most recent, because some manual markdown cleanup work will need to be done, and I think we should focus on the most recent posts for launch and after that work our way back...

benichu · 2015-12-05T17:09:33Z

code reviewer, please assign the PR to yourself...

benichu · 2015-12-05T18:35:41Z

Will also Fix #99

houndci-bot · 2015-12-06T00:58:13Z

spec/controllers/news_item_controller_spec.rb

+      get :index
+    end
+
+    it "populates an array of news_items with the most recently published first" do


Line is too long. [83/80]

mdeloupy · 2015-12-06T21:44:27Z

@montrealrb/organizers I did a quick review, but don't want to take the responsibility of merging this... Anyone else up for it ?

nicholasjhenry · 2015-12-09T14:30:48Z

app/controllers/news_items_controller.rb

-    @news_item = NewsItem.published.find(params[:id])
+    @news_item = news_item_scope.where(slug: params[:slug]).first ||
+                 news_item_scope.where(id: params[:id]).first ||
+                 raise(ActiveRecord::RecordNotFound)


@benichu Could we extract as a method and pushed into the model? e.g.

NewsItem.fetch_published!(params[:id], params[:slug])

This allow to document the method, as I'm unsure why this is required.

Sure. The reason why it's required is to still resolve old wordpress links (ex: if someone clicks on an post url from google)
I'll extract that logic into a tested and documented class method tonight or later today...

Gotcha, we probably should do a redirect to the correct URL, but let's do that as a separate PR.

You are right, just created a ticket for that #151
Until then, I am also going to leave the object loading methods as-is in the controller, I'll just add a comment to the method.

nicholasjhenry · 2015-12-09T14:37:41Z

lib/tasks/database.rake

+      records = YAML.load_file "#{Rails.root}/db/legacy.yml"
+      records.each do |data|
+        news_item = NewsItem.new
+        published_at = data["post_date"]


@benichu Since there's a couple of changes that need to be made, how would you feel call #fetch on these?

data.fetch("post_date")

I like exceptions thrown when the key I expect doesn't exist.

published_at could be nil, if the original post is a draft (which will not happen with the collection I am parsing, as the original query generated only published posts with a date). So it's not really necessary to catch the non-existence of that key.

NOTE: It's a one-off task anyway

Not quite what I meant -- the key should exists, but the value might be nil :-) I should have clarified that this would be for all keys we're accessing in the hash. But happy to move on given it's a one-off task.

nicholasjhenry · 2015-12-09T15:00:29Z

Awesome, @benichu! I ran this locally -- looks like we got us a site :-)

benichu · 2015-12-09T15:15:08Z

Cool, I'll do a couple of the fixes you mentioned and we are good to merge, talk to you later...

benichu · 2015-12-10T00:40:37Z

@nicholasjhenry rebased, I'll merge tomorrow morning.

sophiedeziel · 2015-12-10T05:34:07Z

Haha you have to rebase again 😛

… [WIP] - Generate a wordpress database dump in db/legacy.yml - Create a rake task to import that legacy data in the news_items table - Add some unit and controller specs for NewsItem - Deal with legacy urls: /YYYY/mm/a-wordpress-slug

[chore] Fixes #42 - Import old wordpress data and convert to markdown…

benichu added the in-progress label Dec 3, 2015

benichu self-assigned this Dec 3, 2015

benichu force-pushed the chore-bt-42-import-old-data branch 2 times, most recently from fff419d to 57c4060 Compare December 5, 2015 01:00

houndci-bot reviewed Dec 5, 2015
View reviewed changes

benichu force-pushed the chore-bt-42-import-old-data branch 2 times, most recently from baffaf6 to 2aca4d1 Compare December 5, 2015 17:04

houndci-bot reviewed Dec 5, 2015
View reviewed changes

benichu removed the in-progress label Dec 5, 2015

benichu added this to the Site live for Meetup milestone Dec 5, 2015

benichu removed their assignment Dec 5, 2015

benichu changed the title ~~[WIP] [chore] Fixes #42 - Import old wordpress data and convert to markdown…~~ [chore] Fixes #42 - Import old wordpress data and convert to markdown… Dec 5, 2015

benichu added the requires-code-review label Dec 5, 2015

benichu mentioned this pull request Dec 5, 2015

Cleanup imported and marked as published, wordpress markdown news_items#description #132

Closed

benichu force-pushed the chore-bt-42-import-old-data branch from 2aca4d1 to da082c6 Compare December 6, 2015 00:58

houndci-bot reviewed Dec 6, 2015
View reviewed changes

nicholasjhenry reviewed Dec 9, 2015
View reviewed changes

benichu added requires-amendment and removed requires-code-review labels Dec 9, 2015

benichu self-assigned this Dec 9, 2015

benichu force-pushed the chore-bt-42-import-old-data branch from a96d5f9 to 0806774 Compare December 10, 2015 00:36

benichu removed the requires-amendment label Dec 10, 2015

sophiedeziel added the requires-rebase label Dec 10, 2015

benichu force-pushed the chore-bt-42-import-old-data branch from 0806774 to 3cfb664 Compare December 10, 2015 12:09

benichu removed the requires-rebase label Dec 10, 2015

benichu added a commit that referenced this pull request Dec 10, 2015

Merge pull request #98 from montrealrb/chore-bt-42-import-old-data

9a2f744

[chore] Fixes #42 - Import old wordpress data and convert to markdown…

benichu merged commit 9a2f744 into master Dec 10, 2015

benichu deleted the chore-bt-42-import-old-data branch December 10, 2015 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[chore] Fixes #42 - Import old wordpress data and convert to markdown… #98

[chore] Fixes #42 - Import old wordpress data and convert to markdown… #98

benichu commented Dec 3, 2015

benichu commented Dec 3, 2015

houndci-bot Dec 5, 2015

houndci-bot Dec 5, 2015

benichu commented Dec 5, 2015

benichu commented Dec 5, 2015

benichu commented Dec 5, 2015

houndci-bot Dec 6, 2015

mdeloupy commented Dec 6, 2015

nicholasjhenry Dec 9, 2015

benichu Dec 9, 2015

nicholasjhenry Dec 9, 2015

benichu Dec 10, 2015

nicholasjhenry Dec 9, 2015

benichu Dec 9, 2015

nicholasjhenry Dec 9, 2015

nicholasjhenry commented Dec 9, 2015

benichu commented Dec 9, 2015

benichu commented Dec 10, 2015

sophiedeziel commented Dec 10, 2015

[chore] Fixes #42 - Import old wordpress data and convert to markdown… #98

[chore] Fixes #42 - Import old wordpress data and convert to markdown… #98

Conversation

benichu commented Dec 3, 2015

benichu commented Dec 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benichu commented Dec 5, 2015

benichu commented Dec 5, 2015

benichu commented Dec 5, 2015

Choose a reason for hiding this comment

mdeloupy commented Dec 6, 2015

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

nicholasjhenry commented Dec 9, 2015

benichu commented Dec 9, 2015

benichu commented Dec 10, 2015

sophiedeziel commented Dec 10, 2015