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

Add support for server_env conf #32

Merged
merged 5 commits into from
Nov 25, 2019
Merged

Conversation

simahawk
Copy link
Contributor

No description provided.

Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

@simahawk Great idea!!! but travis is red :-(

@simahawk
Copy link
Contributor Author

@lmignon fixed and:

  • added specific test for param retrieval
  • unified file type handling
  • unified and renamed backend retrieval

@simahawk
Copy link
Contributor Author

ouch, I've left aws_file_acl into the computed fields :/
I'll get back to this PR as #31 is merged

@simahawk
Copy link
Contributor Author

includes #31 now to not forget about new fields

@simahawk
Copy link
Contributor Author

have to solve some weird inheritance issues. I'll look into it

@@ -37,3 +39,33 @@ class StorageBackend(models.Model):
"Public: your file/image can be displayed if nobody is "
"logged (useful to display files on external websites)",
)
url_include_directory_path = fields.Boolean(
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

"backend_id.base_url",
"backend_id.url_include_directory_path",
"relative_path",
)
def _compute_url(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

@simahawk Are we sure that compute methods are triggered if the values provided by the server.env.mixin changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm very good point. @guewen what is the recommended way in this case?
Context: this is a computed + stored field depending on server env params.

Copy link
Contributor

Choose a reason for hiding this comment

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

@simahawk IMO it's not a blocking point. It would probably be necessary to add a warning in the README on this point and propose a procedure to launch this recomputation manually (via click-odoo or ????) if these values are modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure. Can be risky whenever you move a db from prod to integration for instance.
Would be nice to have a common well-known solution.

Copy link
Member

Choose a reason for hiding this comment

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

It kind of defeats the purpose of environment, since a copy of a prod database to another environment will not reset the url.

Does it need to be stored?

Copy link
Contributor Author

@simahawk simahawk Nov 25, 2019

Choose a reason for hiding this comment

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

I don't think so. This computation is not so heavy and is not used so often AFAIR.
OTOH can be done on potentially gazillions of records (images + their scales).

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the point. I share the same POV. The url should no more be stored... If I remember well, into the past, the url field was used by a controller to lookup for the record... it's no more the case and it seems safe to no more store this value into the database.

Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

😍 Thank you @simahawk (Code review)

@simahawk
Copy link
Contributor Author

argh! breaking tests now 😭 Checking...

URL computation depends on server env params now
hence it makes no sense anymore to store its value.

In the past, the url field was used by a controller
to lookup for the record which is not the case anymore.
@simahawk
Copy link
Contributor Author

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 12.0-ocabot-merge-pr-32-by-simahawk-bump-major, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Nov 25, 2019
Signed-off-by simahawk
@lmignon
Copy link
Contributor

lmignon commented Nov 25, 2019

🍾 ❤️ 🎉

@OCA-git-bot OCA-git-bot merged commit a3456f0 into OCA:12.0 Nov 25, 2019
@OCA-git-bot
Copy link
Contributor

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

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.

4 participants