fix: resolving internal links into share spaces and shared files #9821
Conversation
|
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. |
JammingBen
force-pushed
the
fix-internal-share-space-resolving
branch
2 times, most recently
from
c7563e7
to
611cc85
Compare
JammingBen
force-pushed
the
fix-internal-share-space-resolving
branch
2 times, most recently
from
18197bd
to
8c6da63
Compare
| @@ -55,6 +56,7 @@ import { useGroupingSettings } from '@ownclouders/web-pkg' | |||
| import SharesNavigation from 'web-app-files/src/components/AppBar/SharesNavigation.vue' | |||
| import { useGettext } from 'vue3-gettext' | |||
| import { useStore } from '@ownclouders/web-pkg' | |||
| import { useOpenWithDefaultApp } from '../../composables/openWithDefaultApp' | |||
There was a problem hiding this comment.
There also is a index.ts file which is supposed to export all folders in the composables directory. With that it would be possible to import all composables at the same time from the composables folder. Looks like there are more folders missing in the index.ts file - could be cleaned up in a followup, but for the new composable openWithDefaultApp it would be nice if you could use that pattern already. :-)
There was a problem hiding this comment.
Good point, fixed here for the useOpenWithDefaultApp composable. We need to be careful with the imports still, sometimes unit tests require these specific imports to be able to mock stuff and to avoid circular imports.
| space: space | ||
| } | ||
|
|
||
| const defaultEditorAction = getDefaultEditorAction(fileActionsOptions) |
There was a problem hiding this comment.
always nice to see how composables can actually be used in other composables... that turned out nice 💪
There was a problem hiding this comment.
apparently "composables" weren't named that way by accident ;-)
| @@ -103,11 +103,11 @@ Feature: link | |||
| | resource | | |||
| | lorem.txt | | |||
| When "Brian" opens the public link "textLink" | |||
| Then for "Brian" file "shareToBrian.txt" should be selected | |||
| And "Brian" closes the file viewer | |||
There was a problem hiding this comment.
Hm, I get that this is somehow an implicit verification step of being in a file viewer... but reads super weird. What do you think about introducing a generic step for Then "Brian" is in a file viewer or the like? @ScharfViktor any hints here?
There was a problem hiding this comment.
You are right, the open public link step does not contain any checking, and close file viewer just clicks the close button.
need implement step that we can ensure that text editor was opened. A step should be implemented to ensure that the text editor has been opened.
added to my TODO list
There was a problem hiding this comment.
Not necessarily the text editor. I think for a first step it's already sufficient to have a check for any editor / viewer. That's already fulfilled if the close button in the top bar is present. So with the implementation that @JammingBen chose the tests are doing what they should... but they are hard to understand from just reading the Gherkin.
JammingBen
force-pushed
the
fix-internal-share-space-resolving
branch
from
09ceeb2
to
723f54d
Compare
|
Kudos, SonarCloud Quality Gate passed! |
delete-merged-branch
bot
deleted the
fix-internal-share-space-resolving
branch
Description
Aligns the resolving of public and internal links. When
openLinksWithDefaultAppis enabled, links will now always resolve into the default app. Before, this was only working for some links and only under specific circumstances.Related Issue
Types of changes