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

[15.0] [MIG] dms #159

Merged
merged 81 commits into from
May 19, 2022
Merged

[15.0] [MIG] dms #159

merged 81 commits into from
May 19, 2022

Conversation

JasminSForgeFlow
Copy link
Contributor

@JasminSForgeFlow JasminSForgeFlow commented Feb 3, 2022

@JasminSForgeFlow
Copy link
Contributor Author

JasminSForgeFlow commented Feb 3, 2022

Hi @sbidoul,

pre-commit command is failing here because of one CSV file in demo data.

This CSV file is used in the demo data file dms/dms/demo/file.xml.

Gives below error
image

Can you please take a look and help us to solve it?

Thanks

@pedrobaeza
Copy link
Member

Please fix CIs and cherry-pick this recent commit: acfe367

@JasminSForgeFlow
Copy link
Contributor Author

Please fix CIs and cherry-pick this recent commit: acfe367

Hi @pedrobaeza,

cheery-pick is done.

CIs failed because the module depends on
OCA/web#2139
OCA/social#842

and there is one issue coming with pre-commit for one CSV demo data file. So for that require to do changes in pre-commit.

Thanks

@pedrobaeza
Copy link
Member

OK, we have to wait then for the rest to be merged.

@victoralmau
Copy link
Member

Please cherry pick this last change: #161

Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

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

First of all thanks for the contribution.
Some comments:

  • I think it is necessary to squash the administrative commits.
  • It will be necessary to add the change [14.0][FIX] dms: Replace the content field with size in the form view of the directory #161 from v14 (this change and the previous one will need to be placed in order, that is, before the commit "[IMP] dms: black, isort , prettier" to respect the change history).
  • I see that in the changes applied in the commit "Migration to 15.0" many string= are removed from many fields and views, I think this is not necessary and it generates a very large diff that makes the revision difficult.

@JasminSForgeFlow
Copy link
Contributor Author

First of all thanks for the contribution. Some comments:

  • I think it is necessary to squash the administrative commits.
  • It will be necessary to add the change [14.0][FIX] dms: Replace the content field with size in the form view of the directory #161 from v14 (this change and the previous one will need to be placed in order, that is, before the commit "[IMP] dms: black, isort , prettier" to respect the change history).
  • I see that in the changes applied in the commit "Migration to 15.0" many string= are removed from many fields and views, I think this is not necessary and it generates a very large diff that makes the revision difficult.

Hi @victoralmau ,

Changes of #161 have been pulled and squash administrative commit. Please check it.

Regarding string= it's required in v15 to remove otherwise pre-commit test will be failed.

Thanks

Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

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

Good job, thanks for the changes, the code review is OK.
When the dependencies of this addon are merged I will test it in runbot.

@victoralmau
Copy link
Member

Please cherry pick this last change: #166 (before v15 migration commits)

@etobella
Copy link
Member

@JasminSForgeFlow Can you check @victoralmau comments?

@JasminSForgeFlow
Copy link
Contributor Author

JasminSForgeFlow commented May 17, 2022

@JasminSForgeFlow Can you check @victoralmau comments?

Hi @etobella,

I have cherry-pick #166 and also rebased with 15,0

Thanks

@etobella
Copy link
Member

@victoralmau It seems ready for merge. May I kindly ask for a review ??

1 similar comment
@etobella
Copy link
Member

@victoralmau It seems ready for merge. May I kindly ask for a review ??

Copy link
Member

@victoralmau victoralmau left a comment

Choose a reason for hiding this comment

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

Thanks a lot, very good job!!

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 15.0-ocabot-merge-pr-159-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 9a539bd into OCA:15.0 May 19, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 6d635ab. Thanks a lot for contributing to OCA. ❤️

@pedrobaeza
Copy link
Member

/ocabot migration dms

@OCA-git-bot OCA-git-bot added this to the 15.0 milestone May 19, 2022
@OCA-git-bot OCA-git-bot mentioned this pull request May 19, 2022
2 tasks
@LoisRForgeFlow LoisRForgeFlow deleted the 15.0-mig-dms branch October 3, 2022 07:43
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.