Skip to content

[MIG] Migrate auditlog module from 8.0 to 9.0 - #454

Closed
holdenrehg wants to merge 74 commits into
OCA:9.0from
holdenrehg-forks:feature/9.0
Closed

[MIG] Migrate auditlog module from 8.0 to 9.0#454
holdenrehg wants to merge 74 commits into
OCA:9.0from
holdenrehg-forks:feature/9.0

Conversation

@holdenrehg

Copy link
Copy Markdown
Contributor

The majority of the module was already following new api conventions and not using any deprecated methods, so there was only minor clean up to move to over to 9.0

  • Update documentation to point to the new auditlog menu locations. These were changed because the 8.0 version was referencing menus that do not exist in 9.0
  • Change version from 8.0.1.0.0 to 9.0.1.0.0
  • Make the module installable again
  • Remove an unused parameter from pre-migration.py for versioning
  • Update classes to follow OCA new api conventions, camel case instead of snake case
  • Fix typos and remove commented out blocks of code that were irrelevant

- Update documentation to point to the new auditlog menu locations. These were changed because the 8.0 version was referencing menus that do not exist in 9.0
- Change version from 8.0.1.0.0 to 9.0.1.0.0
- Make the module installable again
- Remove an unused parameter from pre-migration.py for versioning
- Update classes to follow OCA new api conventions, camel case instead of snake case
- Fix typos and remove commented out blocks of code that were irrelevant
@oca-clabot

Copy link
Copy Markdown

Hey @holdenrehg, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla
Here is a list of the users:

Appreciation of efforts,
OCA CLAbot

@coveralls

coveralls commented Jun 12, 2016

Copy link
Copy Markdown

Coverage Status

Coverage increased (+3.7%) to 76.271% when pulling a7e708e on holdenrehg:feature/9.0 into 1e935c0 on OCA:9.0.

@holdenrehg

Copy link
Copy Markdown
Contributor Author

Here is a reference to my CLA signature odoo/odoo#12305

@pedrobaeza pedrobaeza mentioned this pull request Jun 12, 2016
59 tasks
@moylop260

Copy link
Copy Markdown
Contributor

@holdenrehg
Thanks for you contribution.

Here is a reference to my CLA signature odoo/odoo#12305

This one is CLA of odoo. Could you sign your CLA of oca https://odoo-community.org/page/cla too?

@moylop260

moylop260 commented Jun 13, 2016

Copy link
Copy Markdown
Contributor

Could you check the license?

  • auditlog/__openerp__.py:22: [C8102(manifest-required-key), ] Missing required key "license" in manifest file

More info here

…ields

These were updated to follow OCA conventions.

- License set to AGPL-3
- Images set to empty array
@holdenrehg

Copy link
Copy Markdown
Contributor Author

@moylop260 Checked and updated the license field. See here for the commit.

@coveralls

coveralls commented Jun 13, 2016

Copy link
Copy Markdown

Coverage Status

Coverage increased (+3.7%) to 76.271% when pulling 233bb87 on holdenrehg:feature/9.0 into 1e935c0 on OCA:9.0.

@moylop260

Copy link
Copy Markdown
Contributor

Thank you
After CLA detail... LGTM

@hbrunn hbrunn added this to the 9.0 milestone Jun 13, 2016
@hbrunn

hbrunn commented Jun 13, 2016

Copy link
Copy Markdown
Member

you should follow the procedure outlined in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-9.0#howto to get 8.0's commits. This allows us to simply cherry pick changes afterwards and keeps history intact.

@holdenrehg

Copy link
Copy Markdown
Contributor Author

Sounds good @hbrunn I'll pull in that 8.0 history and update the PR.

@holdenrehg

Copy link
Copy Markdown
Contributor Author

@hbrunn It appears that all the 8.0 commit history is already there. There was an updated translations file that I pulled from the 12th after I migrated the module.

@hbrunn

hbrunn commented Jun 14, 2016

Copy link
Copy Markdown
Member

you seem to run the commands mentioned above on your current branch, this won't give you the desired results. You need to start out from an upstream 9.0 branch, and then do do the stuff so that the commit history includes all of those: https://github.com/OCA/server-tools/commits/8.0/auditlog

I also see that there are some commits that make the module better, but are not reflected in this PR, like 3f9152c

@holdenrehg

Copy link
Copy Markdown
Contributor Author

@hbrunn So is the proper way to perform the actions on an upstream branch and then merge into this PR? It will add 70+ commits to this PR. Want to make sure I'm handling this the correct way according to OCA conventions.

@hbrunn

hbrunn commented Jun 14, 2016

Copy link
Copy Markdown
Member

yes, that's exactly what we want

Just do all of this in a fresh branch and push overwrite this one. Or just close this PR and create a new one with your new branch.

@holdenrehg

Copy link
Copy Markdown
Contributor Author

Ah thanks @hbrunn

Comment thread auditlog/README.rst

Get the details:

.. image:: /auditlog/static/description/log.png

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I set all these image URLs in my last PR, but it seems they do not work properly on apps.odoo.com (https://apps.odoo.com/apps/modules/8.0/auditlog/). We should change them to relative ones like static/description/IMAGE.png in order to have them displayed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Relative paths do not work when testing. I'll leave these as absolute for now.

Comment thread auditlog/__openerp__.py Outdated
'author': "ABF OSIELL,Odoo Community Association (OCA)",
'license': "AGPL-3",
'website': "http://www.osiell.com",
'license': 'AGPL-3',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@hbrunn

hbrunn commented Jun 14, 2016

Copy link
Copy Markdown
Member

runbot error unrelated

@holdenrehg

Copy link
Copy Markdown
Contributor Author

@hbrunn stripped out that duplicated license key, also thanks for mentioning unrelated runbot errors. Saw the same, related to other modules.

@sebalix

sebalix commented Jun 25, 2016

Copy link
Copy Markdown
Contributor

Tested on Runbot, 👍



def migrate(cr, version):
def migrate(cr):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I did not see this. We should not change the signature of the method here, even if the version parameter is not used. It seems that the migrate method is always called with this parameter: https://github.com/odoo/odoo/blob/9.0/openerp/modules/migration.py#L152

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In fact, this file should be totally removed, as the migration doesn't apply to the version 9.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's right.

_order = "create_date DESC"
_rec_name = 'display_name'

display_name = fields.Char(u"Name", compute="_display_name")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/_display_name/_compute_display_name/g

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

any reason not to overwrite name_get instead? This will be the display name by default, with the added benefit that name_get also returns something sensible. Then you also don't need to set _rec_name. cf https://github.com/OCA/OCB/blob/9.0/openerp/models.py#L1664

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What's better in this scenario? name_get seems to be less elegant if you still keep the computed display_name field. If you get rid of display_name and compute directly in name_get that has worse performance since it will be computed on every view load that has this model defined.

class AuditlogHTTPRequest(models.Model):

    ...

    display_name = fields.Char(u"Name", compute="_compute_display_name")

    @api.multi
    @api.depends('create_date', 'name')
    def _compute_display_name(self):
        for httprequest in self:
            create_date = fields.Datetime.from_string(httprequest.create_date)
            tz_create_date = fields.Datetime.context_timestamp(
                httprequest, create_date)
            httprequest.display_name = u"%s (%s)" % (
                httprequest.name or '?',
                fields.Datetime.to_string(tz_create_date))

    @api.multi
    def name_get(self):
        res = []
        for request in self:
            res += [(request.id, request.display_name)]
        return res

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this a blocking point to migrate on 9.0? At best rename _display_name to _compute_display_name.

@pedrobaeza

Copy link
Copy Markdown
Member

Please rebase the branch to make it mergeable

@sebalix sebalix mentioned this pull request Nov 13, 2016
@pedrobaeza

Copy link
Copy Markdown
Member

Superseeded by #603

@pedrobaeza pedrobaeza closed this Nov 13, 2016
SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (9.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.