feat: forward url path segments to iframe #545
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pReya
changed the title
provokateurin
requested changes
Aug 30, 2023
There was a problem hiding this comment.
This does not work with query parameters and I don't think that will be possible. Still passing paths as a feature is fine.
I saw some weird behaviour when the external site already had a path, because then somehow the id of the site was added to the end of the url. I will have another look once the current comments are fixed.
pReya
changed the title
pReya
changed the title
provokateurin
requested changes
Sep 4, 2023
|
@provokateurin I'm having trouble testing/running the plugin when I use the master branch. What NC version should I use to test/run the master branch? I've tried 27.0.2 and I'm getting incompatibility warnings. Any way to still enable the plugin, even with these warnings? |
|
You need to run the server master branch as well. You can use something like https://github.com/juliushaertl/nextcloud-docker-dev to get that working. |
|
@provokateurin Can you give it another try? I've just tested it with the current Nextcloud master version. Regarding the
If we want to do this, then we could only separate those two cases by not allowing numerical paths as the first path segment on the default route – which would be a very weird and arbitrary limitation. I'd rather not support path segments on the default route at all. It can be clearly documented, that path segments will only work, if the route contains your app ID, and they will not work on the default route. P.S.: Where would you document this feature? In the readme.md file? In the UI? |
provokateurin
requested changes
Sep 4, 2023
|
@provokateurin Anything else I can do to move this along? |
|
Sorry you will have to wait a bit with the next review. I don't work every day and have other things to do as well. |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! |
|
@provokateurin If you ever find the time, could you take another look at this PR? |
|
Sorry I forgot about this. I'll check it again soon |
|
@pReya Can you squash your changes and rebase them against master? This PR includes dependabot commits from master and a merge commit as well which makes it hard to rebase because of conflicts. |
|
I rebased it locally to be able to test the code. If you want I can push it to your branch |
provokateurin
requested changes
Oct 30, 2023
There was a problem hiding this comment.
I finally found why it was so broken for me: When generating URLs for the route you need to pass along the new path parameter:
diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
index afd1309..33e94cc 100644
--- a/lib/AppInfo/Application.php
+++ b/lib/AppInfo/Application.php
@@ -82,7 +82,7 @@ class Application extends App implements IBootstrap {
$href = $site['url'];
if (!$site['redirect']) {
- $href = $url->linkToRoute('external.site.showPage', ['id' => $site['id']]);
+ $href = $url->linkToRoute('external.site.showPage', ['id' => $site['id'], 'path' => '']);
}
return [
diff --git a/lib/BeforeTemplateRenderedListener.php b/lib/BeforeTemplateRenderedListener.php
index 0006519..70c3f92 100644
--- a/lib/BeforeTemplateRenderedListener.php
+++ b/lib/BeforeTemplateRenderedListener.php
@@ -97,7 +97,7 @@ class BeforeTemplateRenderedListener implements IEventListener {
protected function getHref(array $site): string {
if (!$site['redirect']) {
- return $this->urlGenerator->linkToRoute('external.site.showPage', ['id' => $site['id']]);
+ return $this->urlGenerator->linkToRoute('external.site.showPage', ['id' => $site['id'], 'path' => '']);
}
return $site['url'];
diff --git a/lib/Settings/Personal.php b/lib/Settings/Personal.php
index 5721195..a95c187 100644
--- a/lib/Settings/Personal.php
+++ b/lib/Settings/Personal.php
@@ -56,7 +56,7 @@ class Personal implements ISettings {
$url = $quotaLink['url'];
if (!$quotaLink['redirect']) {
- $url = $this->url->linkToRoute('external.site.showPage', ['id' => $quotaLink['id']]);
+ $url = $this->url->linkToRoute('external.site.showPage', ['id' => $quotaLink['id'], 'path' => '']);
}
return new TemplateResponse('external', 'quota', [
|
The changes from #545 (review) are still missing. |
That's why I went for the nullable parameter, b/c then these changes are not necessary. Passing an empty string as a parameter is not very pretty imo. But I'm fine either way, if this is your preferred style. I'd also like to document this feature. But I'm really unsure where to put this, as the |
|
It doesn't matter if you make it nullable or not. The URL generator throws an exception because of the requirement in the route but that one is also necessary to allow it being empty. |
When did you see this exception? What action triggered it? |
|
Just reloading any page on the server lead to an internal server error (I locally reverted the URL generator changes to show this to you): |
Thanks. I'm not sure why I never got that error. But now I was able to do some more (manual) testing and everything seems to work fine. I added some minimal documentation to the Admin section - are you okay with that @provokateurin ? |
|
Documentation looks good, thanks for adding it. You need to remove the changes in l10n/ because that will be done by Transifex. Other than that this PR should be ready to merge |
|
Ah please squash again in the end :) |
Signed-off-by: Moritz Stückler <moritz@bitbetter.de>
|
@provokateurin Done. Sorry for so many rounds of review. The process is all new to me. Thanks for helping me! |
provokateurin
approved these changes
Oct 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
www.mycloud.com/external/1/subpagewill properly forward thesubpagepath segment to your embedded page)closes #479