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 method to set modified_by on contacts #262

Merged
merged 2 commits into from
Apr 9, 2020

Conversation

norkans7
Copy link
Contributor

@norkans7 norkans7 commented Apr 9, 2020

No description provided.

@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #262 into master will increase coverage by 0.04%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
+ Coverage   49.34%   49.39%   +0.04%     
==========================================
  Files          86       86              
  Lines        7885     7896      +11     
==========================================
+ Hits         3891     3900       +9     
- Misses       3382     3383       +1     
- Partials      612      613       +1     
Impacted Files Coverage Δ
models/orgs.go 39.43% <ø> (ø)
web/contact/contact.go 65.35% <66.66%> (+0.06%) ⬆️
models/contacts.go 47.86% <100.00%> (+0.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28ccd5e...4f422d8. Read the comment docs.

@norkans7 norkans7 force-pushed the update-contact-modified-by branch from a02ce35 to e9725e9 Compare April 9, 2020 15:02
// apply modified_by
err = models.UpdateContactModifiedBy(ctx, tx, modifiedContactIDs, request.UserID)
if err != nil {
return nil, http.StatusInternalServerError, errors.Wrapf(err, "error applying modified_by")
Copy link
Member

Choose a reason for hiding this comment

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

Probably should be after the pre commit hooks below.

Copy link
Member

@nicpottier nicpottier left a comment

Choose a reason for hiding this comment

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

Small tweak but otherwise looks good.

@nicpottier
Copy link
Member

So I think this makes sense to do but I wonder whether we should reconsider how we log these types of things. modified_by on Contact really is pretty inaccurate, as a flow may have modified the contact since that was last set, so it may be pointing you in the wrong direction entirely.

Wonder whether the right solution here would rather be to create an audit trail for these events that are presented in the contact history the same as flow events are. That would give you a lot more visibility into what is happening and in truth the logging could be very similar since these modifiers also generate events.

But do think this is fine for the time being. @rowanseymour

@rowanseymour
Copy link
Contributor

Hmm yeah could have a user field on events when they were initiated from the UI.

Similar https://github.com/rapidpro/rapidpro/issues/1016

Also fine with this for now

@norkans7
Copy link
Contributor Author

norkans7 commented Apr 9, 2020

I think it might be useful to have both a user and a flow so the UI could show for each modification what made it a user or a flow or import too

Not sure if we want to have distinction such changed by import or via API by a user

@nicpottier nicpottier merged commit 58ec363 into master Apr 9, 2020
@nicpottier nicpottier deleted the update-contact-modified-by branch April 9, 2020 17:51
rasoro pushed a commit to Ilhasoft/mailroom that referenced this pull request Dec 16, 2024
Switch to official Elastic v8 client library
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.

3 participants