[FIX]do not delete module version if already exists and just update #30

bizzappdev · 2018-10-12T09:04:24Z

If Odoo module version already exists then instead of updating that version it is deleting and creating a new one.

Expected behaviour after this PR, It will not delete the existing version.

Updating the existing version functionality is already there

bizzappdev · 2018-10-12T09:05:29Z

OCA/apps-store#31 will be solved after this PR merged.

bizzappdev · 2018-10-12T09:06:55Z

@RoelAdriaans @oscarolar @StephanRozendaal Can anyone of you please review this.

StephanRozendaal

LGTM

pedrobaeza · 2018-10-12T12:04:50Z

Why Travis is red?

bizzappdev · 2018-10-12T12:08:16Z

@pedrobaeza https://github.com/OCA/interface-github/blob/11.0/github_connector/models/github.py#L75 this is the normal python Class and Travis is expecting a Super call for create method https://github.com/OCA/interface-github/blob/11.0/github_connector/models/github.py#L156

changes are Not from my commit.

StephanRozendaal · 2018-10-12T12:08:41Z

Looks like the flake8 test failed, in the log I see:

github_connector_odoo/models/github_repository_branch.py:88:13: F841 local variable 'module_versions' is assigned to but never used

pedrobaeza · 2018-10-12T12:09:54Z

Yes, you should remove previous line too. But I wonder why it was there initially, as this seems to be put on purpose.

@legalsylvain @cubells any idea?

bizzappdev · 2018-10-12T12:13:23Z

Not needed code removed. thanks @StephanRozendaal
@pedrobaeza No idea why it was there before. might be the original author can answer

legalsylvain

Hi, I understand that it is problematic with the new appstore !

this code was designed to be sure that all the module version was up to date. I mean to handle situations where a module version moved out of the repository.

I think we should replace this code by another, to test if the module still exist, and if not, delete obsolete module version.
please ping me if I'm not clear.

bizzappdev · 2018-10-12T13:45:17Z

@legalsylvain okay clear, You mean to say at the end of the cron job we should call the module and module version verification method.
I will work on that tomorrow

elicoidal · 2018-10-12T14:49:01Z

I think we should replace this code by another, to test if the module still exist, and if not, delete obsolete module version.

Makes sense to me too

legalsylvain · 2018-10-12T14:51:23Z

Not exactly. I mean :
After first analysis :
a repository contains 5 module.version.

when we analyse again, if a module.version disappeared from the repository we should drop it. (and so the appstore should drop the related products also)

so kind of algorithm like :

# at the beginning of the analyse of a repository, save current_module_version_ids

# during the analyse save all the currently analyse module version into a analysed_module_version_ids

# at the end, drop all module versions that are in current_module_version_ids - analysed_module_version_ids

(something like that)

elicoidal · 2018-10-12T15:04:14Z

should drop the related products also

you mean inactivate. When it reappears we just reactivate the product.

@hbrunn has an interesting idea that we could develop later to create products in Alpha maturity level for open PR with green travis. We should be able to activate/inactivate a version after discovery IMO

bizzappdev · 2018-10-15T10:36:39Z

Not exactly. I mean :
After first analysis :
a repository contains 5 module.version.

when we analyse again, if a module.version disappeared from the repository we should drop it. (and so the appstore should drop the related products also)

so kind of algorithm like :
# at the beginning of the analyse of a repository, save current_module_version_ids

# during the analyse save all the currently analyse module version into an analysed_module_version_ids

# at the end, drop all module versions that are in current_module_version_ids - analysed_module_version_ids
(something like that)

this seems little tricky as we do process always all the branches. we just project the branches which have state to_analyze.
IMO we should have another special cron job to check the drop odoo module version. which we can enhance in the app-store module to archive the product.

elicoidal · 2018-10-15T10:59:38Z

IMO we should have another special cron job to check the drop odoo module version. which we can enhance in the app-store module to archive the product.

Looks like a good idea to one specific cron job to have one pass on all module and change the active field indeed

bizzappdev · 2018-10-15T15:49:09Z

as per that, I have added a new cron job for cleaning up the Odoo module version.
@legalsylvain Can you please review

legalsylvain

Nice design, thanks !

A blocking point about path. LGTM otherwise.

regards.

legalsylvain · 2018-10-15T16:54:52Z

github_connector_odoo/models/odoo_module_version.py

+    def clean_odoo_module_version(self):
+        for module_version in self:
+            module_ver_path = os.path.join(
+                module_version.repository_branch_id.local_path,


You should base your analyse about local_path, but on module_paths. Otherwise, analysis of project base on Odoo. (OCB, OpenUpgrade, will fail).
so a for each is required.

see line 87 (previously 94) of the same file.

legalsylvain · 2018-10-15T16:56:14Z

github_connector_odoo/models/odoo_module_version.py

+                module_version.technical_name)
+            if os.path.exists(module_ver_path):
+                continue
+            module_version._process_clean_module_version()


~~why not just move_version.unlink() ? and removing the function _process_clean_module_version.~~
(sorry for the noice, just understood that point, reading https://github.com/OCA/apps-store/pull/36/files#diff-65d1a23272a69d00ef4e64370b596546R10

elicoidal

I have a question (I am not able to see it in the code sorry).
I understand the pseudo code as followed:

if a module is present we check whether there is a product associated (active or inactive)
if product is inactive -> activate and publish
if module is not present and there is a product -> inactivate and unpublish

I think this is what you do here: https://github.com/OCA/apps-store/pull/36/files but I just want to be sure

bizzappdev · 2018-10-16T06:16:36Z

@elicoidal yes the flow is exactly what you say.
in interface-github only the code is for deleting the module version while on apps-store module we have code for achieving the product.

bizzappdev · 2018-10-16T06:17:15Z

@legalsylvain changes are done as per your suggestion. and module_paths is Now supported.

legalsylvain · 2018-10-16T10:29:42Z

and module_paths is not supported.

I don't understand that point. I'm running this module in local instance, and I analyse correctly modules, downloading the code of https://github.com/odoo/odoo and the analysis of the modules of Odoo core works fine.

bizzappdev · 2018-10-16T10:34:03Z

and module_paths is not supported.

I don't understand that point. I'm running this module in local instance, and I analyse correctly modules, downloading the code of https://github.com/odoo/odoo and the analysis of the modules of Odoo core works fine.

Sorry for the previous update. "Now" is autocorrected in mind by "Not". and that has changed the complete meaning of the Sentence.

legalsylvain · 2018-10-16T11:30:14Z

OK. ;-).
note : I don't see the changes, did you commited ?

bizzappdev · 2018-10-16T11:53:37Z

indeed push was not done.

legalsylvain

minor non blocking point. LGTM. code review, no test.

Thanks for the changes.

legalsylvain · 2018-10-16T12:50:51Z

github_connector_odoo/models/odoo_module_version.py

+        for module_version in self:
+            # Compute path(s) to analyze
+            branch = module_version.repository_branch_id
+            if branch.module_paths:


Hi thanks.
Not blocking point. (can be done later).
it could be great to refactor this lines because duplicated with a new function in repository.branch model named :

@api.multi def _get_module_paths: self.ensure_one() branch = self if branch.module_path paths = [] for path in branch.module_paths.split('\n'): if path.strip(): paths.append(os.path.join(branch.local_path, path)) else: paths = [branch.local_path] return paths

Instead, I would suggest having a new functional field.

sorry my bad it will be not possible as we might have list in paths

As you wish on that point.

it can be the function, (as proposed).

it can be a new computed field my_computed_field = fields.Char(help="coma separated path") (as in ADDONS_PATH syntax), that we have just to split after.
no clear point of view on that point.

removed redundant code and added function for that.

bizzappdev · 2018-10-17T08:53:28Z

@StephanRozendaal can you please review again because few things are changed after your approval.

elicoidal

LGTM

elicoidal · 2018-10-25T02:35:26Z

@StephanRozendaal are we good to go?

StephanRozendaal

LGTM

elicoidal · 2018-10-25T09:03:20Z

thanks @StephanRozendaal merging

…CA#30) * [FIX]do not delete module version if already exists and just update * [FIX]remove not needed code * [IMP] added clean odoo module version cron * [IMP]support module_paths also * [IMP]separate function fo the module path to remove redundant code

[FIX]do not delete module version if already exists and just update

24ec38b

bizzappdev mentioned this pull request Oct 12, 2018

Module missing manifest information OCA/apps-store#31

Closed

StephanRozendaal approved these changes Oct 12, 2018

View reviewed changes

[FIX]remove not needed code

abbf67b

legalsylvain requested changes Oct 12, 2018

View reviewed changes

[IMP] added clean odoo module version cron

3787728

bizzappdev mentioned this pull request Oct 15, 2018

[IMP] while clean the odoo module version archive the product OCA/apps-store#36

Merged

legalsylvain requested changes Oct 15, 2018

View reviewed changes

elicoidal reviewed Oct 15, 2018

View reviewed changes

[IMP]support module_paths also

c32a175

legalsylvain approved these changes Oct 16, 2018

View reviewed changes

[IMP]separate function fo the module path to remove redundant code

c76a7e9

elicoidal approved these changes Oct 17, 2018

View reviewed changes

elicoidal added the needs review label Oct 17, 2018

elicoidal added this to the 11.0 milestone Oct 17, 2018

StephanRozendaal approved these changes Oct 25, 2018

View reviewed changes

elicoidal merged commit 8fe4283 into OCA:11.0 Oct 25, 2018

[FIX]do not delete module version if already exists and just update #30

[FIX]do not delete module version if already exists and just update #30

Conversation

bizzappdev commented Oct 12, 2018

bizzappdev commented Oct 12, 2018

bizzappdev commented Oct 12, 2018

StephanRozendaal left a comment

Choose a reason for hiding this comment

pedrobaeza commented Oct 12, 2018

bizzappdev commented Oct 12, 2018

StephanRozendaal commented Oct 12, 2018

pedrobaeza commented Oct 12, 2018

bizzappdev commented Oct 12, 2018

legalsylvain left a comment

Choose a reason for hiding this comment

bizzappdev commented Oct 12, 2018

elicoidal commented Oct 12, 2018

legalsylvain commented Oct 12, 2018 • edited Loading

elicoidal commented Oct 12, 2018

bizzappdev commented Oct 15, 2018

elicoidal commented Oct 15, 2018

bizzappdev commented Oct 15, 2018

legalsylvain left a comment

Choose a reason for hiding this comment

legalsylvain Oct 15, 2018

Choose a reason for hiding this comment

legalsylvain Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

elicoidal left a comment

Choose a reason for hiding this comment

bizzappdev commented Oct 16, 2018

bizzappdev commented Oct 16, 2018 • edited Loading

legalsylvain commented Oct 16, 2018

bizzappdev commented Oct 16, 2018

legalsylvain commented Oct 16, 2018

bizzappdev commented Oct 16, 2018

legalsylvain left a comment

Choose a reason for hiding this comment

legalsylvain Oct 16, 2018

Choose a reason for hiding this comment

bizzappdev Oct 16, 2018

Choose a reason for hiding this comment

bizzappdev Oct 16, 2018

Choose a reason for hiding this comment

legalsylvain Oct 16, 2018

Choose a reason for hiding this comment

bizzappdev Oct 16, 2018

Choose a reason for hiding this comment

bizzappdev commented Oct 17, 2018

elicoidal left a comment

Choose a reason for hiding this comment

elicoidal commented Oct 25, 2018

StephanRozendaal left a comment

Choose a reason for hiding this comment

elicoidal commented Oct 25, 2018

legalsylvain commented Oct 12, 2018 •

edited

Loading

legalsylvain Oct 15, 2018 •

edited

Loading

bizzappdev commented Oct 16, 2018 •

edited

Loading