12.0 mig auditlog #1392

GoodERPJeff · 2018-10-14T13:02:21Z

No description provided.

… module

…file

… ones to make overridding easier

…ther minor fixes

…itlog.log' model (standard 'create_date' field is used instead)

…S.txt file removed

…t never used

…ail data - if any - are migrated during the installation)

… a field is deleted (e.g. migration)

… 'field_description' fields changed into related fields

elicoidal

LGTM
Some remarks based on new standard (most of them nice to have).
no technical review

auditlog/README.rst

auditlog/__manifest__.py

elicoidal · 2018-10-14T15:10:15Z

auditlog/models/log.py

+        'ir.model.fields', ondelete='cascade', string="Field", required=True)
+    log_id = fields.Many2one(
+        'auditlog.log', string="Log", ondelete='cascade', index=True)
+    old_value = fields.Text("Old Value")


technically no need for string anymore if the name is same as column name

I don't think this is good practic

elicoidal · 2018-10-14T15:10:52Z

auditlog/views/auditlog_view.xml

@@ -0,0 +1,201 @@
+<?xml version="1.0" encoding="utf-8"?>
+<odoo>


add copyright in all XML files

can you give me some example to add copyright in XML?
I never see MIS builder nor module_auto_update get this lines.

It's a bit opinionated, some add, some don't. Usually it's the same as in code files, just inside . I'm pro-copyright in XML - it's still sort-of code and licensed thing

auditlog/views/auditlog_view.xml

elicoidal · 2018-10-14T15:13:45Z

auditlog/__manifest__.py

+        'views/auditlog_view.xml',
+        'views/http_session_view.xml',
+        'views/http_request_view.xml',
+    ],


add image key as cover to improve the visibility in the apps store

have to leave it to next PR, I am not good at icon design :)

elicoidal · 2018-10-14T15:21:09Z

You can try to improve test coverage based on the codecov report

GoodERPJeff · 2018-10-15T02:08:18Z

actrually we found some issue on this module, about read audit. what I concern is should we finish all the tasks(migration, bugfix, coverage) for a module in same PR or focus on migration first then do other parts. I totally agree test coverage is very important. will try our best to catch up.

elicoidal · 2018-10-15T08:44:21Z

It will depend and is a bit up to you. First PR could be the mandatory part and a second one for additional functions or improvements but sometimes it can make sense (or more simple) to do it at once.
If unclear ask but in principle just common sense will do just fine
@GoodERPJeff

chienandalu

@GoodERPJeff What's the state of this PR?

sebalix

Minor comment/suggestion, but the commit e04431b should be removed, I don't know why it's there actually.

sebalix · 2018-11-30T14:36:54Z

auditlog/views/auditlog_view.xml

-    <menuitem id="menu_action_auditlog_rule_tree" parent="menu_audit" action="action_auditlog_rule_tree"/>
+    <menuitem id="menu_action_auditlog_rule_tree"
+              parent="menu_audit"
+              action="action_auditlog_rule_tree"/>


Is there a lint error about this? If not, we must try to avoid change to make commits backport easier.

sebalix · 2018-11-30T14:38:50Z

auditlog/README.rst

@@ -1,4 +1,4 @@
-.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg
+.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.png


Would you use the new README structure? https://github.com/OCA/maintainer-tools/tree/master/template/module
It would be wonderful 😄

@GoodERPJeff it would be great to have this split as mentioned above

sebalix · 2018-11-30T14:45:10Z

@GoodERPJeff About the bug when logging read calls, it's here since a long time (Odoo 8.0), don't bother with this in this PR, having the module migrated is enough at first.

chienandalu

Overall looks ok (waiting for runbot to test) but the commit history is really messed up. Please fix it to keep a clean history.

alexey-pelykh

Please enhance tests coverage

alexey-pelykh · 2019-02-08T11:30:31Z

auditlog/views/auditlog_view.xml

+                            <tree>
+                                <field name="field_description"/>
+                                <field name="field_name"/>
+                                <!--<field name="old_value"/>-->


This comment is not needed

alexey-pelykh · 2019-02-08T11:30:40Z

auditlog/views/auditlog_view.xml

+                                <field name="field_name"/>
+                                <!--<field name="old_value"/>-->
+                                <field name="old_value_text"/>
+                                <!--<field name="new_value"/>-->


Also not needed

alexey-pelykh · 2019-02-08T11:32:04Z

auditlog/views/auditlog_view.xml

@@ -0,0 +1,201 @@
+<?xml version="1.0" encoding="utf-8"?>
+<odoo>


It's a bit opinionated, some add, some don't. Usually it's the same as in code files, just inside . I'm pro-copyright in XML - it's still sort-of code and licensed thing

alexey-pelykh · 2019-02-08T11:32:23Z

auditlog/tests/test_autovacuum.py

@@ -0,0 +1,47 @@
+# © 2016 ABF OSIELL <https://osiell.com>


Copyrights should be reformatted as

# Copyright <YEAR(S)> <AUTHOR(S)>

alexey-pelykh · 2019-02-08T11:33:38Z

auditlog/README.rst

@@ -1,4 +1,4 @@
-.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg
+.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.png


@GoodERPJeff it would be great to have this split as mentioned above

BT-fgarbely · 2019-02-21T15:59:37Z

Hello @GoodERPJeff

What is the current state of this auditlog migration?
I need this module in v12 too. It would be great if you could make it mergeable.

Thanks in advance.

EvgeniyERP · 2019-02-25T15:29:52Z

Hello @GoodERPJeff

What is the current state of this auditlog migration?
I need this module in v12 too. It would be great if you could make it mergeable.

Thanks in advance.

Same question from me :)
Is there any ETA: when the module will be available for v12?
Waiting for it with passion. Ready to run some tests on our system (Odoo Enterprise on Premise, v12).

pedrobaeza · 2019-02-25T15:32:26Z

@EvgeniyERP you can put this PR in your system, try, and provide feedback. This is the main goal of a PR: being tested by reviewers.

murtuzasaleh · 2019-04-02T11:32:09Z

auditlog/models/http_session.py

+        If no HTTP user session is available, returns `False`.
+        """
+        if not request:
+            return False


@GoodERPJeff Remove if not request condition and use if request condition, if not request it will return False as per method.

@murtuzasaleh don't if it's worth to change this existing code, it is also to avoid an indentation level.

murtuzasaleh · 2019-04-02T11:37:08Z

auditlog/models/http_request.py

+        first call.
+        If no HTTP request is available, returns `False`.
+        """
+        if not request:


Same as above comment.

murtuzasaleh · 2019-04-02T11:40:23Z

auditlog/models/rule.py

+              "updated, but it is slower)\n"
+              "Fast log: only log the changes made through the create and "
+              "write operations (less information, but it is faster)"))
+    # log_action = fields.Boolean(


Please remove it if it is not used.

murtuzasaleh · 2019-04-02T11:51:51Z

@GoodERPJeff If possible can you please make fields readonly after do Subscribe in auditlog.rule because when we change model after Subscribe it will create a previous model log and current model log.
Settings << Technical << Audit << Rules

pedrobaeza · 2019-04-09T14:33:43Z

Superseeded by #1556

Syncing from upstream OCA/server-tools (10.0)

sebalix and others added 30 commits October 13, 2018 14:21

[ADD] Module 'auditlog' - A substitute to the deprecated 'audittrail'…

85546be

… module

[FIX] Module 'auditlog' - Fix some flake8 errors

f8a41a7

[FIX] Module 'auditlog' - Put the module description in a README.rst …

87bf703

…file

[ADD] Module 'auditlog' - 'auditlog.pot' file added

610846f

[IMP] Module 'auditlog' - Split the 'create_logs()' method in several…

df303bf

… ones to make overridding easier

[IMP] Module 'auditlog' - Clean up vim lines

65c63e5

[IMP] Module 'auditlog' - Add 'name' attributes on some filters and o…

3f48b6d

…ther minor fixes

[IMP] Module 'auditlog' - Removed the 'timestamp' field from the 'aud…

711b616

…itlog.log' model (standard 'create_date' field is used instead)

[IMP] Module 'auditlog' - README.rst updated (based on the OCA template)

29df5a4

[FIX] Module 'auditlog' - 'time' module imported but unused

6e9785d

[IMP] ignore rules for nonexisting objects

ff0b7d7

[ADD] init hook to migrate from audittrail

d73d1c5

[FIX] also migrate id sequences

7b7019c

[IMP] Module 'auditlog' - Contributors updated in README.rst + AUTHOR…

e30c204

…S.txt file removed

[ADD] change xmlids of legacy rules to point to auditlog.rule

6273d82

[IMP] use new import of _, SUPERUSER_ID

f1e4dcb

[ADD] allow overriding modules to pass additional log values

8824b42

[IMP] cache model and field ids

f57bc32

[IMP] don't make a temporary copy of dict keys

34176be

[FIX] don't reset caches in register_hook

9d23a0b

[ADD] pass old_values to create_log when deleting a record

81f3c9e

[FIX] cope with no additional log values

e2c4921

[ADD] tests

ce62cab

[FIX] Module 'auditlog' - local variable 'ir_model' is assigned to bu…

23b6cae

…t never used

[IMP] use a model for test that is not influenced by the mail module

7c5cd9a

[FIX] Module 'auditlog' - Remove 'active' field from __openerp__.py

b4bb515

[IMP] Module 'auditlog' - README.rst updated (inform that the audittr…

223b8e3

…ail data - if any - are migrated during the installation)

[IMP] Module 'auditlog' - Useless lines of code removed

123f03e

[IMP] Module 'auditlog' - Autoremove 'auditlog.log.line' records when…

d739093

… a field is deleted (e.g. migration)

[IMP] Module 'auditlog' - Model 'auditlog.log.line', 'field_name' and…

1647289

… 'field_description' fields changed into related fields

mig pr for auditlog

e04431b

GoodERPJeff mentioned this pull request Oct 14, 2018

Migration to version 12.0 #1368

Closed

32 tasks

elicoidal added the needs review label Oct 14, 2018

elicoidal added this to the 12.0 milestone Oct 14, 2018

elicoidal suggested changes Oct 14, 2018

View reviewed changes

This was referenced Oct 14, 2018

12.0 mig base cron exclusion #1393

Closed

12.0 mig base tecnical user #1394

Closed

elicoidal mentioned this pull request Oct 14, 2018

12.0 mig base exception #1395

Closed

chienandalu reviewed Nov 12, 2018

View reviewed changes

[MIG]some detail change follow CR result

ec7a9ef

sebalix requested changes Nov 30, 2018

View reviewed changes

chienandalu suggested changes Dec 3, 2018

View reviewed changes

alexey-pelykh suggested changes Feb 8, 2019

View reviewed changes

max3903 requested review from murtuzasaleh and osi-scampbell March 27, 2019 00:42

murtuzasaleh suggested changes Apr 2, 2019

View reviewed changes

pedrobaeza mentioned this pull request Apr 5, 2019

12.0 mig auditlog #1551

Closed

sebalix mentioned this pull request Apr 9, 2019

[MIG] auditlog: Migration to 12.0 #1556

Merged

pedrobaeza closed this Apr 9, 2019

SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024

Merge pull request OCA#1392 from OCA/10.0

486d05b

Syncing from upstream OCA/server-tools (10.0)

		@@ -0,0 +1,201 @@
		<?xml version="1.0" encoding="utf-8"?>
		<odoo>

		@@ -1,4 +1,4 @@
		.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg
		.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.png

Uh oh!

12.0 mig auditlog #1392

12.0 mig auditlog #1392

Uh oh!

Conversation

GoodERPJeff commented Oct 14, 2018

Uh oh!

elicoidal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elicoidal commented Oct 14, 2018

Uh oh!

GoodERPJeff commented Oct 15, 2018

Uh oh!

elicoidal commented Oct 15, 2018

Uh oh!

chienandalu left a comment

Choose a reason for hiding this comment

Uh oh!

sebalix left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebalix commented Nov 30, 2018

Uh oh!

chienandalu left a comment

Choose a reason for hiding this comment

Uh oh!

alexey-pelykh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BT-fgarbely commented Feb 21, 2019

Uh oh!

EvgeniyERP commented Feb 25, 2019

Uh oh!

pedrobaeza commented Feb 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murtuzasaleh commented Apr 2, 2019