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

feat: add option to update modified timestamp #29174

Closed
wants to merge 1 commit into from

Conversation

iamejaaz
Copy link
Member

Closes frappe/erpnext#44330 and frappe/crm#492

Please provide enough information so that others can review your pull request:

frappe/erpnext#43968

Explain the details for making this change. What existing problem does the pull request solve?

  • Add fields in system setting
    image
  • When new communication is received, then update the modified timestamp.
  • When a new comment is added, update the modified timestamp.

no-docs

@iamejaaz iamejaaz requested a review from nabinhait January 15, 2025 07:01
@github-actions github-actions bot added the add-test-cases Add test case to validate fix or enhancement label Jan 15, 2025
@ankush
Copy link
Member

ankush commented Jan 15, 2025

This should be per-doctype and not system settings. See gameplan's implementation of this. https://github.com/frappe/gameplan/blob/605e73d1db4908554f3e4bd60a96787d6c8dc081/gameplan/gameplan/doctype/gp_discussion/gp_discussion.json#L69

modified isn't indexed by default nor do we sort by modified by default. The meaning of modified shouldn't be changed just because a few apps want to treat it differently, those apps can add last_active if that's desired.

This is no different than ERPNext having a separate posting_date instead of just overriding creation. These timestamps serve very important role in traceability.

@ankush ankush closed this Jan 15, 2025
@@ -59,6 +59,10 @@ def after_insert(self):
notify_mentions(self.reference_doctype, self.reference_name, self.content)
self.notify_change("add")

# update modified timestamp of reference document
if frappe.db.get_single_value("System Settings", "update_timestamp_on_new_comment"):
Copy link
Member

Choose a reason for hiding this comment

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

Side note: Use frappe.get_system_settings (it's cached by default)

@@ -59,6 +59,10 @@ def after_insert(self):
notify_mentions(self.reference_doctype, self.reference_name, self.content)
self.notify_change("add")

# update modified timestamp of reference document
if frappe.db.get_single_value("System Settings", "update_timestamp_on_new_comment"):
frappe.get_doc(self.reference_doctype, self.reference_name).update_modified()
Copy link
Member

Choose a reason for hiding this comment

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

Entire doc shouldn't be fetched to update one field.

@ankush
Copy link
Member

ankush commented Jan 15, 2025

@iamejaaz can you discuss with @shariquerik and think of CRM specific fix?

@ankush
Copy link
Member

ankush commented Jan 15, 2025

Also, these kinds of weird settings will get out of hand very quickly, are you really gonna recommend anyone who wants this behavior to toggle it from system settings?

If it's worth having then it should be worth having by default in CRM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add-test-cases Add test case to validate fix or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug:Last Modified Should be updated when new Activity received Sorting leads #43968
3 participants