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

feat: include a provider option to allow custom app names #10335

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jvillafanez
Copy link
Member

Description

Introduce the Provider option to handle some differences between web office products. This will be used instead of the Name. Note that the Name will still be used in other places such as service names, and it will be shown in the web interface.

The intention is to provide a bit of extra branding: you could have a specific Collabora installation fully customized and branded to your liking, so you could use BBox as Name and keep Collabora as Provider.

Note: There are some issues with the naming used as Name, so it's highly recommended to use only alphanumeric chars. Spaces are known to cause issues.

Related Issue

#10306

Motivation and Context

We need to distinguish between the name and the provider.

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Notes

Need to consider a migration, otherwise it could be a breaking change.
For installations with Collabora, this change shouldn't cause any issue because the default provider will also be Collabora. However, for other installations, the provider will also be Collabora despite connecting to an OnlyOffice installation (for example). This might cause some features to break.

We also need changes in reva for the new templates. It's currently using the app name (which might be different) instead of the provider name. Right now, the collaboration service can't sent the provider name to reva.

Copy link

update-docs bot commented Oct 17, 2024

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@butonic
Copy link
Member

butonic commented Oct 17, 2024

Too many things are called provider, already. Can we use Product? IMO that better conveys product specific peculiarities. I don't think being able to deploy multiple versions of the same product makes sense, not even in demo installations so we don't need a generic provider property. If we really want to distinguish providers we can introduce an ID property, but for now I prefer to only add Product.

Copy link

sonarcloud bot commented Oct 18, 2024

@jvillafanez
Copy link
Member Author

Changed Provider to Product.

I don't think being able to deploy multiple versions of the same product makes sense, not even in demo installations

You could have OnlyOffice 7 running and you could try / test the integration with OnlyOffice 8. Having both running at the same time would allow you to switch to the old version if the new one is having problems.
Allowing your users to use different versions would allow you to gather feedback on the new version, wait for critical bugs (at least for your users) to be fixed in the new version, or even give your users time to adapt to the new version before switching down the old one.
While it might not make sense for small version upgrades (we don't expect big changes for the users), it seems reasonable for mayor version upgrades which could bring more breaking changes.

In theory, having 2 wopi containers with app names "OnlyOffice7" and "OnlyOffice8" pointing to their corresponding OnlyOffice servers should work. Unless we want to officially support this setup, it isn't probably worthy to test it, but I'd rather not to block this option if possible.

@butonic
Copy link
Member

butonic commented Oct 18, 2024

Yes, I see that use case as well. But it would still be tha same app product: onlyofffice. The app name would be different OnlyOffice 7 and OnlyOffice 8. I am voting to degrade app name to be just a displayname, which is currently not the case. IIRC the appregistry relies on the app name to find the right proider. Which I think still works when we treat it as a display name. But the collaboration service as an app provider needs to be able to take different codepaths based on the product.

Hm 🤔

the collaboration service is its own wopi service and is always tied to a product. We could make this a service config option. Then we would not need to change the CS3 api.

Right? What am I missing?

@jvillafanez
Copy link
Member Author

In theory, the PR should work in that scenario once it's finished.

In any case, I think we need reva to be aware of the product due to how the office templates are implemented there. Using the name won't be enough because it could be any name for any known product.
I don't know what are the planned changes to "de-hardcode" the product names in reva, but I don't see a way to send that information right now with the current CS3 API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants