AO3-6960 Admin post drafts and previews#5822
Conversation
github-actions
Bot
added
Has Migrations
pmonfort
left a comment
pmonfort
left a comment
There was a problem hiding this comment.
Great work! The code looks good. Tested locally and the core flow works well. Left a few comments, mostly small things.
| end | ||
| @admin_posts = @admin_posts.order('created_at DESC').page(params[:page]) | ||
| @page_subtitle = t(".page_title") | ||
| @admin_posts = @admin_posts.posted.order(published_at: :desc).page(params[:page]) |
There was a problem hiding this comment.
This uses will_paginate, but drafts below uses pagy. Should this also use pagy for consistency?
| if @admin_post.save | ||
| flash[:notice] = ts("Admin Post was successfully created.") | ||
| redirect_to(@admin_post) | ||
| if params[:preview_button] |
There was a problem hiding this comment.
Should we validate that the post is valid before previewing? Something similar to what's done in update where @admin_post.valid? is checked before previewing the post.
| @admin_post = AdminPost.find(params[:id]) | ||
| authorize @admin_post | ||
|
|
||
| @admin_post.posted = true |
There was a problem hiding this comment.
Should we also check @admin_post&.translated_post&.draft? like we do in create and update to avoid potential bugs?
There was a problem hiding this comment.
I've added a validation to the model to prevent translations from being posted first, which should cover this and also prevent already published posts from turned into translations of a draft post.
There was a problem hiding this comment.
Great approach, thanks for taking care of that!
| <%= span_if_current t(".ao3_news"), admin_posts_path %> | ||
| </li> | ||
| <li> | ||
| <%= span_if_current t(".ao3_news_drafts"), drafts_admin_posts_path %> |
There was a problem hiding this comment.
The "AO3 News Drafts" link is shown without a permission check. The ticket says it should be visible to "any admin with a role listed in the AdminPostPolicy section." Should this be wrapped in <% if policy(AdminPost).can_draft? %> like the other links?
| <% end %> | ||
|
|
||
| <% if @preview_mode %> | ||
| <li><%= form.submit t(".actions.edit"), name: "edit_button" %> |
There was a problem hiding this comment.
Missing </li> closing tag here.
| @admin @comments | ||
| Feature: Admin Actions to Draft News | ||
| In order to post news items | ||
| As an an admin |
| t.boolean :posted, default: false, null: false | ||
| t.datetime :published_at, default: nil, null: true | ||
| end | ||
| AdminPost.update_all("published_at = created_at, posted = 1") |
There was a problem hiding this comment.
Won't this need to happen before the migration happens to avoid the migration failing due to the null columns? Although, I'd be more in favour of moving this to a before rake tasks (iirc the file doesn't exist but it might after a recent PR of mine) since we can test it with rspec that way
| _german_post = create(:admin_post, :draft, language: german, translated_post: english_post) | ||
| finnish_post = create(:admin_post, language: finnish, translated_post: english_post) | ||
| _indonesian_post = create(:admin_post, :draft, language: indonesian, translated_post: english_post) |
There was a problem hiding this comment.
Small nitpick: you can omit the variable assignment for german/indonesian, since they are not used
| end | ||
| end | ||
|
|
||
| describe "#set_published_at" do |
There was a problem hiding this comment.
Could you also add a test for the validation that posting a translation isn't possible when the parent post is in draft?
| translations.find_each do |post| | ||
| post.update(posted: true, published_at: self.published_at) | ||
| end |
There was a problem hiding this comment.
Any reason not to use update_all here, so we have a single batch-update query?
| <dd class="published"><%= admin_post.created_at %></dd> | ||
| <% if admin_post.posted? %> | ||
| <dt class="published"><%= t(".published") %></dt> | ||
| <dd class="published"><%= admin_post.published_at %></dd> |
There was a problem hiding this comment.
Shouldn't this be wrapped in <%= l(...) %> so it can be localized?
| </div> | ||
| <div class="clear"><!-- presentational --></div> | ||
|
|
||
| <%= form_for(@admin_post) do |f| %> |
There was a problem hiding this comment.
Could you replace this with form_with? Per https://guides.rubyonrails.org/form_helpers.html#using-form-tag-and-form-for, that's the standard for Rails 5.1+
3 participants
Issue
https://otwarchive.atlassian.net/browse/AO3-6960
Purpose
Allows admins to save unpublished (draft) admin posts and preview changes before publication.
Testing Instructions
Manual QA steps TBD.
Credit
marcus8448 (he/him)