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

add public-activity for tracking updates in publishable records #119

Merged
merged 10 commits into from
Nov 25, 2019

Conversation

kowal
Copy link
Contributor

@kowal kowal commented Nov 18, 2019

Summary

  • Adds public_activity page
  • tracks Company, Geography, Legislation, Litigation, Target resources
  • recognizes changes to visibility_status, deletes and all other "edits"
  • only single updates for now (bulk updates skips callbacks)

act200

@tsubik
Copy link
Contributor

tsubik commented Nov 19, 2019

What should we do with bulk upload and bulk actions? Just one activity (disable that callback), or bunch of activities for each update. Something to consider.

@simaob
Copy link
Contributor

simaob commented Nov 20, 2019

So on the other PR I mentioned a single activity, but I guess it will depend on how much they use it, maybe for now I'd keep with tracking all of the activities individually and if see how it looks. Otherwise we might need to tweak the display so that they can see exactly what was affected.

@simaob
Copy link
Contributor

simaob commented Nov 20, 2019

One thing to add is the link to the resource, so that it's easy to go to that page and check. Other than that, this is what I wanted =D We might want to add pagination to the page, in case this grows quite large.

@tsubik
Copy link
Contributor

tsubik commented Nov 21, 2019

This page could be a standard active admin index page, right? With filters, pagination, everything. Last activities could be another section that we will add to the dashboard.

@kowal
Copy link
Contributor Author

kowal commented Nov 21, 2019

I'll get back to this tomorrow.
@tsubik good point with standard active admin index page, will check that.

@kowal kowal force-pushed the feature/public-activity branch 4 times, most recently from 74bcab5 to 1bfe537 Compare November 24, 2019 14:19
@kowal
Copy link
Contributor Author

kowal commented Nov 24, 2019

What should we do with bulk upload and bulk actions? Just one activity (disable that callback), or bunch of activities for each update. Something to consider.

Bulk updates are currently implemented using update_all which skips validations & callbacks, so we won't record any activity automatically. We should get more info of what we really want here and update batch actions classes accordingly but maybe let's do it i another story.

@kowal kowal marked this pull request as ready for review November 24, 2019 15:17
@kowal kowal changed the title [WIP] add public-activity for tracking updates in publishable records add public-activity for tracking updates in publishable records Nov 24, 2019
@kowal kowal requested review from tsubik and simaob November 24, 2019 15:33
@kowal
Copy link
Contributor Author

kowal commented Nov 24, 2019

@simaob @tsubik please review.

I won't have much time during upcoming week, so if you find some small things to fix, feel free to take this over! thanks ;)

@kowal
Copy link
Contributor Author

kowal commented Nov 24, 2019

rebased after #131 was merged

@tsubik
Copy link
Contributor

tsubik commented Nov 24, 2019

CSV import will generate as many activities as there were updates, maybe to be consistent bulk update should also do the same? I'm not sure.


return unless errors.blank? && previous_changes.present?

create_activity activity_key_for_last_update, owner: owner
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to manually create activities (for bulk updates, CSV imports) we could extract from here something like #create_activity_with_current_changes:

module PublicActivityTrackable
  # ..
  def create_activity_with_current_changes!
    # ..
    create_activity activity_key_for_last_update, owner: owner
  end

  protected

  def track_updates_activity
    yield
    create_activity_with_current_changes!
  end
end

then for mentioned cases:

company.update_attributes()
company.create_activity_with_current_changes!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could just start without using update_all in batch actions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just update and everything should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've done in this way. Looks like it works as it should now I think

Copy link
Contributor

@tsubik tsubik left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@simaob simaob merged commit 22e4f87 into develop Nov 25, 2019
@simaob simaob deleted the feature/public-activity branch November 25, 2019 10:34
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.

None yet

3 participants