[16] fs_storage : Allow to choose different method to check connection

[florian-dacosta]

Since #320, in order to resolve some cache issues, the way to check connection did change and became mandatory (which was not the case before)

Now, a marker file is created on remote server which is then read each time we need to connect.
I did ran into some issues because of this on server with limited access rights.. (Not allowed to write in the remote server for instance).
Another issue is that some partners don't expect Odoo to send these kind of files (the marker files) and their system is not able to ignore it for instance.

I find this way quite intrusive, when connecting to remote server that are managed by external partners.
We could have to deal with a server with read-only access, and this is not possible anymore.

To solve this issue, I propose to give an option on the fs storage on how the connection should be checked.
By default, the mark file stuff is set, but you could choose or implement other methods it this one does not work for your case.

I did implement the one we had before (list file on the whole folder), but I think it is valid to implement still other methods if necessary.

@hparfr @lmignon @JordiMForgeFlow @sebalix @nilshamerlinck

[JordiMForgeFlow]

For me it looks good to have the option to decide, I would just maybe recommend to add some description in the README on the implications of using each method.

[florian-dacosta]

I am not sure we want to go into such detail in the README, but at least a help on the field to describe each option would be nice indeed

[lmignon]

Thank you for this improvment @florian-dacosta . Can you add a paragraph into the usage.rst file to explain the check connection mechanism and when to use one or an other.

[florian-dacosta]

For me it looks good to have the option to decide, I would just maybe recommend to add some description in the README on the implications of using each method.

Done @JordiMForgeFlow

[lmignon]

Why do you pass the forced method into the context? It could be an optional argument of the test_config method...

Suggested change

	return self.storage_id.with_context(
	force_connection_method=self.check_connection_method
	)._test_config()
	return self.storage_id._test_config(force_check_method=self.check_connection_method)

[florian-dacosta]

It is because I did not want to pass an arg to fs property method. And connection is checked by calling the fs property...
And I did not especially want to change the way connection is checked.

[lmignon]

but you could also call

self.storage_id._check_connection_with_method(self.storage_id.fs, self.check_connection_method)

The same effect and no need to use the context to pass an args... I prefer explicit way to pass args...

[florian-dacosta]

Well yes, I could, this will then call _check_connection_with_method twice actually :
Once because of self.storage_id.fs, which will call the _check_connection_with_method and the second time because we call it directly.
I don't want to duplicate the code of _test_config so it behave differently in case it is called from wizard or from a fs.storage with a check_connection_methodconfigured...

Then I see 2 options if we really get rid of the context :

1. Always call _check_connection_with_method in case of manual check, it will check twice if the fs.storage is configured to have a method but it is no big deal since it only happens on manual action (which is rarely done)
2. When checking the connection manually, we always open the wizard, even if the check_connection_method is set.
   I would rather not, because it adds an extra step and it give the choice to choose a different check_connection_method in the wizard than the one configured on the fs.storage...

I also prefer explicit way to pass args, but it seemed like the easiest way to avoid too much code duplication/addition.

Anyway, I am not against the 1 I did a commit toward this way.

[lmignon]

IMP We don't need to pass args via context

Suggested change

	check_connection_method = self.check_connection_method or self.env.context.get(
	"force_connection_method", ""
	)
	return self._check_connection_with_method(fs, check_connection_method)
	return self._check_connection_with_method(fs, self.check_connection_method)

[florian-dacosta]

The same method is used when connection is checked from the wizard, for a fs.storage with empty check_connection_method

[lmignon]

We don't need to use the context to pass args from wizard. We can directly call the _check_connection_with_method from the wizard.

[florian-dacosta]

The call on _test_config instead of _check_connection_with_method from the wizard is there to use the same notification mechanism.

[lmignon]

Thank you for this great improvement. LGTM (Code review)

[lmignon]

@JordiMForgeFlow can you take a look at last changes?

[JordiMForgeFlow]

It looks good to me 👍🏼

[lmignon]

/ocabot merge minor

[OCA-git-bot]

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-357-by-lmignon-bump-minor, awaiting test results.

[OCA-git-bot]

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