feat: 🎸 methods and db changes to publish single map and batch-publish multiple maps (no interface yet) #1772

PeculiarE · 2022-06-29T13:10:49Z

Fixes #1771

PR is descriptively titled 📑 and links the original issue above 🔗
tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
code is in uniquely-named feature branch and has no merge conflicts 📁
screenshots/GIFs are attached 📎 in case of UI updation
ask @publiclab/mapknitter-reviewers for help, in a comment below

gitpod-io · 2022-06-29T13:10:56Z

app/controllers/spam_controller.rb

codeclimate · 2022-06-29T13:11:26Z

Code Climate has analyzed commit 0eec279 and detected 4 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	4

View more on Code Climate.

codecov · 2022-06-29T13:12:22Z

Codecov Report

Merging #1772 (7bfa104) into main (833eb37) will increase coverage by 0.70%.
The diff coverage is 100.00%.

❗ Current head 7bfa104 differs from pull request most recent head fd1bc22. Consider uploading reports for the commit fd1bc22 to get more accurate results

@@            Coverage Diff             @@
##             main    #1772      +/-   ##
==========================================
+ Coverage   64.72%   65.43%   +0.70%     
==========================================
  Files          40       40              
  Lines        1219     1244      +25     
==========================================
+ Hits          789      814      +25     
  Misses        430      430

Impacted Files	Coverage Δ
app/controllers/application_controller.rb	`91.66% <100.00%> (ø)`
app/controllers/spam_controller.rb	`100.00% <100.00%> (ø)`
app/models/map.rb	`91.92% <100.00%> (+0.10%)`	⬆️
app/models/user.rb	`100.00% <100.00%> (ø)`

TildaDares · 2022-06-29T18:22:45Z

app/controllers/spam_controller.rb

+    @map = Map.find(params[:id])
+    if publish_check(@map)
+      notice_text = 'Map published.'
+      notice_text.chop! << ' and author unbanned.' if unban_check(@map)


I like the use of #chop here!

Thank you 😄

yes, very nice!!!

TildaDares · 2022-06-29T18:27:33Z

app/controllers/spam_controller.rb

+        unbanned_authors << map.user.id if unban_check(map)
+      end
+    end
+    flash[:notice] = helpers.pluralize(published_maps, 'map') + ' published and ' + helpers.pluralize(unbanned_authors.size, 'author') + ' unbanned.'


Since we're only using the unbaned_authors variable for pluralization, maybe we should keep count of the unbanned authors instead of the authors themselves.

I'm sorry @TildaDares, but could you please explain further as I'm not exactly sure what to change? I am keeping count of the unbanned authors and not the total number of authors.

Sorry for not clarifying. I mean the unbanned_authors is currently storing an array of unbanned author's ids. Since we're only using the unbanned_authors variable for pluralization, we don't actually need the unbanned author's object. We only need to keep count of how many authors are being unbanned just like you did for published_maps.

Yeah, I totally get now. Made the change. Does this work fine?

Yes this works. Thanks @PeculiarE!

TildaDares · 2022-06-29T18:32:05Z

app/controllers/application_controller.rb

@@ -53,7 +53,7 @@ def logged_in?

  def logged_in_as(roles, action)
    unless current_user && roles.any? { |role| current_user.role == role }
-      flash[:error] = "Only #{roles.collect { |role| role.pluralize }.join(' and ')} can #{action}."
+      flash[:error] = "Only #{roles.collect(&:pluralize).join(" and ")} can #{action}."


TildaDares · 2022-06-29T18:38:03Z

@PeculiarE Seems like Rubocop has a few complaints about the comments. Not really important.

PeculiarE · 2022-06-29T21:02:27Z

@PeculiarE Seems like Rubocop has a few complaints about the comments. Not really important.

Yeah. I've fixed it though in the latest commit I just pushed.

jywarren

Just a thought on method naming. If you prefer to just leave as-is, let me know! Otherwise are we ready to merge? Thank you!!!

jywarren · 2022-06-30T10:45:22Z

app/controllers/spam_controller.rb

      map.user.ban unless map.anonymous? || map.user.status == User::Status::BANNED
    end
+
+    def publish_check(map)


Do you think it's worth using a longer method name that reads more like human speech, like check_and_publish? Just an idea. The comments make it clear but i'm thinking when it's used in other files.

Yeah, it's definitely worth it. I'm all for longer method-names for better readability. Will use your suggestions and re-push. Thank you ❤️

jywarren · 2022-06-30T10:45:50Z

app/controllers/spam_controller.rb

+    @map = Map.find(params[:id])
+    if publish_check(@map)
+      notice_text = 'Map published.'
+      notice_text.chop! << ' and author unbanned.' if unban_check(@map)


yes, very nice!!!

jywarren · 2022-06-30T10:46:39Z

test/models/user_test.rb

+
+    user.unban
+    assert_equal User::Status::NORMAL, user.status
+    assert user.status_updated_at.to_f >= old_time.to_f


Love all the tests, very thorough. Thank you!

Thank you 😄 !

PeculiarE · 2022-06-30T13:48:24Z

Just a thought on method naming. If you prefer to just leave as-is, let me know! Otherwise are we ready to merge? Thank you!!!

I've renamed the methods, and I believe we're ready to merge 😄...Thank you!

jywarren · 2022-07-01T11:47:06Z

Looks super, thank you!!!

…h multiple maps (no interface yet) (publiclab#1772) * feat: 🎸 publish single map and batch-publish multiple maps * style: 💄 implemented code style suggestions & refactored routes * refactor: 💡 discarded authors' object and used count instead * refactor: 💡 made method names more readable

feat: 🎸 publish single map and batch-publish multiple maps

0eec279

codeclimate bot reviewed Jun 29, 2022

View reviewed changes

app/controllers/spam_controller.rb Show resolved Hide resolved

app/controllers/spam_controller.rb Show resolved Hide resolved

app/controllers/spam_controller.rb Show resolved Hide resolved

PeculiarE self-assigned this Jun 29, 2022

PeculiarE added feature spam outreachy labels Jun 29, 2022

PeculiarE added this to the Outreachy: MapKnitter.org Spam Management System milestone Jun 29, 2022

PeculiarE added the in-progress label Jun 29, 2022

style: 💄 implemented code style suggestions & refactored routes

1ae3abf

PeculiarE requested review from jywarren, cesswairimu and TildaDares June 29, 2022 17:11

PeculiarE mentioned this pull request Jun 29, 2022

Outreachy: "MapKnitter.org Spam Management System" Project Q&A #1745

Open

TildaDares reviewed Jun 29, 2022

View reviewed changes

refactor: 💡 discarded authors' object and used count instead

e23d056

PeculiarE requested a review from TildaDares June 29, 2022 21:05

jywarren reviewed Jun 30, 2022

View reviewed changes

refactor: 💡 made method names more readable

fd1bc22

PeculiarE removed the in-progress label Jun 30, 2022

PeculiarE requested a review from jywarren June 30, 2022 14:00

jywarren merged commit 0d2e311 into publiclab:main Jul 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 🎸 methods and db changes to publish single map and batch-publish multiple maps (no interface yet) #1772

feat: 🎸 methods and db changes to publish single map and batch-publish multiple maps (no interface yet) #1772

PeculiarE commented Jun 29, 2022 •

edited

Loading

gitpod-io bot commented Jun 29, 2022

codeclimate bot commented Jun 29, 2022

codecov bot commented Jun 29, 2022 •

edited

Loading

TildaDares Jun 29, 2022

PeculiarE Jun 29, 2022

jywarren Jun 30, 2022

TildaDares Jun 29, 2022 •

edited

Loading

PeculiarE Jun 29, 2022

TildaDares Jun 29, 2022

PeculiarE Jun 29, 2022

TildaDares Jun 30, 2022

TildaDares Jun 29, 2022

PeculiarE Jun 29, 2022

jywarren Jun 30, 2022

TildaDares commented Jun 29, 2022

PeculiarE commented Jun 29, 2022

jywarren left a comment

jywarren Jun 30, 2022

PeculiarE Jun 30, 2022

jywarren Jun 30, 2022

jywarren Jun 30, 2022

PeculiarE Jun 30, 2022

PeculiarE commented Jun 30, 2022

jywarren commented Jul 1, 2022

feat: 🎸 methods and db changes to publish single map and batch-publish multiple maps (no interface yet) #1772

feat: 🎸 methods and db changes to publish single map and batch-publish multiple maps (no interface yet) #1772

Conversation

PeculiarE commented Jun 29, 2022 • edited Loading

gitpod-io bot commented Jun 29, 2022

codeclimate bot commented Jun 29, 2022

codecov bot commented Jun 29, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TildaDares Jun 29, 2022 • edited Loading

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

Choose a reason for hiding this comment

TildaDares commented Jun 29, 2022

PeculiarE commented Jun 29, 2022

jywarren left a 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

PeculiarE commented Jun 30, 2022

jywarren commented Jul 1, 2022

PeculiarE commented Jun 29, 2022 •

edited

Loading

codecov bot commented Jun 29, 2022 •

edited

Loading

TildaDares Jun 29, 2022 •

edited

Loading