Tested on Damken-Cloud, it works! 👍

Feature wise this looks ok.

We should add tests for this.

• Unit tests
• Acceptance tests

Depending Tasks

• Check editors
• Fix editors or open ticket

@phil-davis Can you help / give pointers how to create an acceptance test step?

Feature wise this looks ok.

We should add tests for this.

• Unit tests
• Acceptance tests

Depending Tasks

• Check editors
• Fix editors or open ticket

Checked (fixed editors):

• files_texteditor
• files_pdfviewer
• richdocuments

@phil-davis
Requirements for the ui acceptance test would be:

1. Adding a before test hook which enables apps "files_pdfviewer" and "richdocuments"
2. Add a PDF file to the test user's storage
3. Click on the PDF File asserts there is a context menu shown with two items for the individual apps
4. Click on one of the items will open the file in the selected app (not sure if this is part of acceptance test)
5. Adding a after test hook which tears down "files_pdfviewer" and "richdocuments"

Is this addressable with the current test framework?

Tested on Damken-Cloud, it works!

You mentioned some issues with your mobile device, can you recheck?

Is this addressable with the current test framework?

Yes, in the sense that "almost anything" is computable. For example, we have some test scenarios in core that are run with the notifications app installed and enabled.

Instead of installing 2 extra apps during core test CI, we could put the test into richdocuments CI, where it can install files_pdfviewer and then check that the 2 options are available and work.

It will be even easier if there are 2 apps that can open text files. files_texteditor and ? Then its extra-easy for the tests to double-check the expected text in the displayed file.

Is this addressable with the current test framework?

Yes, in the sense that "almost anything" is computable. For example, we have some test scenarios in core that are run with the notifications app installed and enabled.

Instead of installing 2 extra apps during core test CI, we could put the test into richdocuments CI, where it can install files_pdfviewer and then check that the 2 options are available and work.

It will be even easier if there are 2 apps that can open text files. files_texteditor and ? Then its extra-easy for the tests to double-check the expected text in the displayed file.

I think collabora/richdocuments can also open text files but I don't think there is another more handy reader in owncloud repos, however there is a simple third party app:
https://github.com/miqrogroove/files_textviewer

I still think we need to ensure that the 2 apps are only concurrently installed during this specific test, because I don't know if other tests might fail ?

I still think we need to ensure that the 2 apps are only concurrently installed during this specific test, because I don't know if other tests might fail ?

Yes, the test scenarios would be put in a separate test suite that would run in its own pipeline. That stops it from effecting other tests.

I still think we need to ensure that the 2 apps are only concurrently installed during this specific test, because I don't know if other tests might fail ?

Yes, the test scenarios would be put in a separate test suite that would run in its own pipeline. That stops it from effecting other tests.

Great, do you think we could pair on this next week?

When I checkout this branch and make, composer takes me back quite a long way. It would be nice to get it up-to-date with current master, just to be sure that is works fine with all the recently-merged things.

@JammingBen can you rebase and force-push? Or is it OK for me to do it?

@JammingBen can you rebase and force-push? Or is it OK for me to do it?

@phil-davis I could do it on Monday. Feel free to do it if you need it earlier though.

@JammingBen owncloud/files_texteditor#331 (comment)

@phil-davis
We added the new WebUI-tests as part of this PR, I think you already talked to Jan yesterday regarding this decision.

The tests run fine on my local machine as I have both the Collabora and the Files Textviewer app installed. In Drone they still fail, I assume because those apps are not installed during the test run? Could you give us a hint how to achieve that?

https://drone.owncloud.com/owncloud/core/27974/128/15

@JammingBen this should do something useful: fc36fe0

There was already a pipeline that runs with notifications app installed, and the starlark has "generic" code in it that "knows" how to clone an app from GitHub and...

Let's see how CI goes.

https://drone.owncloud.com/owncloud/core/27977/152/10 install-extra-apps worked.
https://drone.owncloud.com/owncloud/core/27977/152/14 the 2 scenarios passed - good stuff

I did:

1. refactored fileActionsMenu.feature to create Alice without bothering to have a full set of skeleton files for her, and to put the upload of lorem.txt in to the Background section.
2. Use assertStringContainsString instead of assertEquals to get improved error message - if there is an error then assertEquals spits out "Failed asserting that true matches expected false.
   " which is not really helpful for a developer who wants to know what went wrong.
   Now if I force a fail by changig the expected text, the message is like:

    Then the file actions menu should be displayed with these items on the webUI            # WebUIFileActionsMenuContext::theFileActionsMenuShouldBeDisplayed()
      | Open in Text Editor | Open in Colabora |
      WebUIFileActionsMenuContext::theFileActionsMenuShouldBeDisplayed Item 'Open in Colabora' was not found.
      Failed asserting that 'Details Rename Download Lock file Open in Collabora Open in Text Editor Delete' contains "Open in Colabora".

After this is merged, we can think if we want to add a test scenario in files_texteditor that has richdocuments also enabled, and actually clicks on "Open in Text Editor" and checks that the file content is actually visible in the text editor.

The code here LGTM. Please get review from developers...

@phil-davis Awesome, thanks for your support here!

After this is merged, we can think if we want to add a test scenario in files_texteditor that has richdocuments also enabled, and actually clicks on "Open in Text Editor" and checks that the file content is actually visible in the text editor.

Sounds good to me. We already put some work into this yesterday, so it shouldn't be a big deal.

@JammingBen @DeepDiver1975 in many cases having two editors allowed will cause everything go bananas, and is misconfiguration. Exception ofc are PDFs, and for this case it is valid dialog usecase

Having here collabora and files_texteditor and a third party textviewer works smooth so far for txt files

@janackermann @JammingBen yes, but you need proper locking (that is not implemented in viewers), so editing concurrently from many editors works properly. You cannot safely edit the same document and the same time using two different editors

@mrow4a Agree but this is not the point of this PR using different collaboration suites makes always trouble.

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

Confirmed in 10.7.0RC1

• Example.odt can choose Collabora or Office Online (when both are installed)
• ownCloud Manual.pdf can choose between Collabora or ffiles_pdfviewer (when...)
  But the 'Open in Collabora' text as sketched in the initial comment here , is just an 'Edit' -> https://github.com/owncloud/core/issues/38544

@jnweiger
Please Check
owncloud/richdocuments#370