Moderators audit logs

[brarnaudovski]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Documentation Update

Description

Wrapper classes are created under Audit namespace, which are able to use the internal Rails Instrumentation API

To be done:

• Documentation
• Definition of Event payloads

Related Tickets & Documents

Fixes #3141

Added to documentation?

• docs.dev.to
• readme
• no documentation needed

[lightalloy]

Looks good to me.
There are test failures on Travis. They are not necessarily related to your changes, but please, look into them to be sure.

[rhymes]

Sorry @brarnaudovski I didn't leave a note with the review! The code look solid! I've added a few comments

[brarnaudovski]

Thank you @rhymes and @lightalloy for reviewing this. I'll try to resolve some of the comments and make changes to this PR

[brarnaudovski]

Hi. I am having strange behavior in travis CI. Acording to build, it fails with:

rspec ./spec/services/credits/ledger_spec.rb:22 # Credits::Ledger returns a list of user purchases with their costs
rspec ./spec/services/notifications/reactions/send_spec.rb:60 # Notifications::Reactions::Send when a reaction is persisted and has siblings creates a notification with the correct json
rspec ./spec/services/notifications/reactions/send_spec.rb:89 # Notifications::Reactions::Send when a reaction is persisted and has siblings when notification exists updates the notification
rspec ./spec/services/notifications/reactions/send_spec.rb:96 # Notifications::Reactions::Send when a reaction is persisted and has siblings when notification exists updates the notification json

But when I make local tests, tests are not failing.

$ rspec spec/services/notifications/reactions/send_spec.rb spec/services/credits/ledger_spec.rb

and the output

The HashDiff constant used by this gem conflicts with another gem of a similar name.  As of version 1.0 the HashDiff constant will be completely removed and replaced by Hashdiff.  For more information see https://github.com/liufengyun/hashdiff/issues/45.
[Zonebie] Setting timezone: ZONEBIE_TZ="Port Moresby"
.......................

Finished in 4.45 seconds (files took 3.6 seconds to load)
23 examples, 0 failures

Please help!

[maestromac]

Let me re-run it for you to check if this is a flaky test

[lightalloy]

@brarnaudovski I'll look at it on Monday.

[lightalloy]

These tests fail if you run all the services specs: bundle exec rspec spec/services/
I'll look deeper into the problem.

[lightalloy]

@brarnaudovski the failures are not related to your changes. We prepared a separate pr to fix them #3518

[brarnaudovski]

Hi. I notice re-occurrence of flaky test at user_edits_an_article_spec.rb
Please see -> https://travis-ci.com/thepracticaldev/dev.to/builds/120490310

Also I can reproduce locally when running rspec spec/system/articles/
Sometime tests are failing, sometimes are OK. And when failing, they fail on random test:

1) Editing with an editor user clicks the edit button
     Failure/Error: expect(page).to have_current_path(link + "/edit")
       expected "/settings" to equal "/username3/sample-article-16d7/edit"
 # ./spec/system/articles/user_edits_an_article_spec.rb:16:in `block (2 levels) in <main>'

1) Editing with an editor user unpublishes their post
     Failure/Error: fill_in "article_body_markdown", with: template.gsub("true", "false")

     Capybara::ElementNotFound:
       Unable to find field "article_body_markdown" that is not disabled

     # ./spec/system/articles/user_edits_an_article_spec.rb:36:in `block (2 levels) in <main>'

There is some try to fix this. See this PR

@maestromac Are you able to look more into this?

[lightalloy]

Thanks for adding a test. I left a couple of suggestions/questions related to the test and behavior.
I think we are close to merging a feature :)

[lightalloy]

@brarnaudovski I left a comment on the original issue in case you want to continue contributing to this feature #3141 (comment)

[lightalloy]

Thanks for the changes, I'm approving it.
I think that remove_moderator and add_moderator should be separate log records, and I'll leave it as an opportunity for future improvement.