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

sort published drafts by last updated #9995

Merged
merged 2 commits into from
Aug 21, 2021
Merged

Conversation

TildaDares
Copy link
Member

Fixes #9985

@gitpod-io
Copy link

gitpod-io bot commented Aug 5, 2021

@codecov
Copy link

codecov bot commented Aug 5, 2021

Codecov Report

Merging #9995 (1055cfe) into main (f41f8e7) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 1055cfe differs from pull request most recent head ce374d1. Consider uploading reports for the commit ce374d1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #9995      +/-   ##
==========================================
+ Coverage   82.13%   82.19%   +0.05%     
==========================================
  Files          98       98              
  Lines        5968     5971       +3     
==========================================
+ Hits         4902     4908       +6     
+ Misses       1066     1063       -3     
Impacted Files Coverage Δ
app/controllers/home_controller.rb 98.66% <100.00%> (ø)
app/controllers/images_controller.rb 70.27% <100.00%> (ø)
app/controllers/notes_controller.rb 85.09% <100.00%> (+0.22%) ⬆️
app/helpers/application_helper.rb 87.62% <100.00%> (+2.21%) ⬆️
app/services/search_service.rb 95.00% <100.00%> (-0.10%) ⬇️
app/controllers/user_tags_controller.rb 83.78% <0.00%> (+1.35%) ⬆️

@TildaDares
Copy link
Member Author

@jywarren None of the tests are failing

@noi5e
Copy link
Contributor

noi5e commented Aug 5, 2021

Nice @TildaDares!

Is there a way we can get this tested in some way before merging? Screenshots, or even with a scrubbed database? That seems important for the original issue at #9985.

@ebarry
Copy link
Member

ebarry commented Aug 10, 2021

Thank you for writing this @TildaDares !!!!

@jywarren
Copy link
Member

We discussed using Timecop on the weekly call, i think that could be a nice test -- maybe a functional test? Or integration test?

@jywarren
Copy link
Member

This is awesome though, thank you!!!

@TildaDares
Copy link
Member Author

We discussed using Timecop on the weekly call, i think that could be a nice test -- maybe a functional test? Or integration test?

@jywarren Is it how notes on the activity feed are sorted I'm supposed to test? Currently, I'm testing this by changing the timestamp of an old note to a time far in the future so that it will appear as the first item in the activity feed but this line sorts the items in the activity feed by newly created.

activity = (notes + wikis + comments + answer_comments).sort_by(&:created_at).reverse

@jywarren
Copy link
Member

So as to the test, it looks like we actually have one here!

test 'draft author can publish the draft' do
perform_enqueued_jobs do
UserSession.create(users(:jeff))
node = nodes(:draft)
old_created = node['created']
old_changed = node['changed']
assert_equal 3, node.status
ActionMailer::Base.deliveries.clear
Timecop.freeze(Date.today + 1) do
get :publish_draft, params: { id: node.id }
assert_response :redirect
assert_equal "Thanks for your contribution. Research note published! Now, it's visible publicly.", flash[:notice]
node = assigns(:node)
assert_equal 1, node.status
assert_not_equal old_changed, node['changed'] # these should have been forward dated!
assert_not_equal old_created, node['created']
assert_equal 1, node.author.status
assert_redirected_to '/notes/' + users(:jeff).username + '/' + (Time.now).strftime('%m-%d-%Y') + '/' + node.title.parameterize
email = ActionMailer::Base.deliveries.last
assert_equal '[PublicLab] ' + node.title + " (##{node.id}) ", email.subject
end
end
end

But are we not testing enough? We want to:

  1. create a draft
  2. confirm the draft has current timestamp
  3. move time forward more than 1 day
  4. publish the draft
  5. confirm draft has new (current day + one day) timestamp
  6. confirm draft has new URL too

It does look like that line 155 is sorting a second time by created_at, but for nodes, created_at is just a function which formats the timestamp, i believe? see def created_at in node.rb to confirm.

Hope this helps!

assert_equal 1, node.status
end

assert_not_equal revision_timestamp, node.latest['timestamp']
Copy link
Member

Choose a reason for hiding this comment

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

Here could we assert what it should be equal to? Also, we could assert the wrong (original) timestamp before we publish the draft, to show that it was different before vs after.

This looks great! Thanks Tilda!!!

Copy link
Member Author

Choose a reason for hiding this comment

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

@jywarren I've made the changes

@codeclimate
Copy link

codeclimate bot commented Aug 21, 2021

Code Climate has analyzed commit 6ac3a98 and detected 0 issues on this pull request.

View more on Code Climate.

@jywarren
Copy link
Member

This is fantastic. Great job!!!!

@jywarren jywarren merged commit d741da5 into publiclab:main Aug 21, 2021
@TildaDares TildaDares deleted the sort-drafts branch August 24, 2021 15:02
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
* sort published drafts by last updated

* added functional test
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
* sort published drafts by last updated

* added functional test
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.

Refine chronological display of published notes that were previously saved as drafts
4 participants