8000 Double slash problem on card pop-ups · Issue #785 · wekan/wekan · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Double slash problem on card pop-ups #785

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

Closed
xet7 opened this issue Feb 6, 2017 · 19 comments
8000 Closed

Double slash problem on card pop-ups #785

xet7 opened this issue Feb 6, 2017 · 19 comments
Labels
Milestone

Comments

@xet7
Copy link
Member
xet7 commented Feb 6, 2017

Moved here from wefork#49

Summary

The card's pop-up links to URL like:
http://192.168.30.240/wekan//b/Gtw5WWes4kvanupHu/devel/HduAsvhiCJLPJF4Hb

Note the double slash //b, if I remove one slash, it works as expected. Also, navigating to the boards doesn't suffer from this.

This issue affects Wekan running behind webserver (Apache/Nginx/Caddy) proxy. Wekan only works when using locally without proxy, for example like http://localhost:3000 .

@Serubin tried to get that FlowRouter fix kadirahq/flow-router#691 merged, with process like forking multiple other repos that would then be hosted under Wekan GitHub organization, but was not able to make Wekan build process working correctly.

For more details, see previous discussion at wefork#49

@Serubin
Copy link
Contributor
Serubin commented Feb 6, 2017

My initial attempt was to update the remote flow_router repo to my own repo named serubin:flow_router. This however required me to update every subsequent repo that used any version of flow_router. I also ran into some odd dependency issues and decided it wasn't worth the effort.

My second attempt (which I've only just begun and am working on fixing up some meteor environment issues) is to add kadira:flow_router as a local package and let meteor use that over the remote versions. meteor hasn't been able to see my local package yet, but I'm still working to fix this.

@xet7 xet7 added this to the 2016 milestone Feb 11, 2017
@rubberduck203
Copy link

I'd be happy to pitch in anyway I can. I spent most of the day trying to come up with an Nginx rewrite that would fix it and failed so far.

I'm unfamiliar with meteor and flow router, but I'm wondering if it's possible to get off of the call to absoluteUri() and use a relative path in the view instead.

@Serubin
Copy link
Contributor
Serubin commented Feb 20, 2017

Currently the most realistic thing to do seems to be adding my fork of flowrouter to a "local package."

The flow router fix requires two packages, one of the main branch and another of the ssr branch. flowrouter separates the two for someone reason and requires both.

So far I've been unable to get meteor to see my local packages though. I've been dealing with some other stuff in life and haven't had time to investigate further into this issue.

@Serubin
Copy link
Contributor
Serubin commented Feb 20, 2017

The current issue with flow-router is this:
I went through and added some logging to the FlowRouter.url() function and found this output:

  var completePath = this.path.apply(this, arguments);
  // /wekan/b
  var basePath = this._basePath || '/';
  // /wekan
  var pathWithoutBase = completePath.replace(new RegExp('^' + basePath), '');
  // /b
  return Meteor.absoluteUrl(pathWithoutBase);
  // https://mydomain.net/wekan//b

The router should definitely be taking out the leading slash.

Changing line 7 of lib/router.js of flowrouter to

var pathWithoutBase = completePath.replace(new RegExp('^(\/)|' + basePath), '');

An argument could be added for keeping that leading slash if necessary (ie. using a hashbang).

@Marabon
Copy link
Marabon commented Mar 17, 2017

@Serubin I've tried to get your fork working, but I'm running into issues with it. I've spent some time debugging and I think I know where it's failing.
It works fine if your URL is domain.tld or domain.tld:port, however if you want to have wekan at a URL like this: domain.tld/wekan it seems to fail.
Using your fork with domain.tld/wekan would end up with domain.tld/wekan/wekan/b/3k3j42... when clicking a card to display card details.

I haven't been able to figure out how to make it work, but I'm sure you'll be able to.

@Serubin
Copy link
Contributor
Serubin commented Mar 17, 2017

@Marabon That sounds about right. I made a mistake in my branch.

var pathWithoutBase = completePath.replace(new RegExp('^' + basePath + '(\/)'), '');

Feel free to fix, I'm without my laptop (due to repairs) for the moment so I wont be able to get that working just yet.
I have yet to get this properly merged into wekan, if you have any suggestions on how to make that happen I'd love to hear.

@Marabon
Copy link
Marabon commented Mar 18, 2017

Thanks, that new line works perfectly with my scenario of domain.tld/wekan!

@rubberduck203
Copy link

That's fantastic news!

I've got to mention it though... it's not great to rely on a fork like this. I did try to get attention of the FlowRouter folks, but it does seem to be unmaintained at this point. Are there other options to replace it? I'm happy to open a new issue if y'all feel it's more appropriate.

@quimnuss
Copy link
quimnuss commented Mar 18, 2017 via email

@Serubin
Copy link
Contributor
Serubin commented Mar 18, 2017

@Marabon Are you using my version of the router in wekan?

@rubberduck203 The idea is to move it over to the wekan namespace once everything is working correctly again.

@Marabon
Copy link
Marabon commented Mar 18, 2017

@Serubin I couldn't get it to work simply adding your flow-router and removing the kadira one so I ended up cloning the kadira one to a local packages dir and simply applying your fix.
It would also work if I cloned yours and renamed the package to kadira, else it would say it was missing it.
Your last fix is working perfectly though so thank you for that.

@Serubin
Copy link
Contributor
Serubin commented Mar 19, 2017

I'll edit my repo shortly. I'm going to see if I can replicate what you did on my end and get that fix working and merged shortly

@marcelofpfelix
Copy link

Thank you so much Serubin!!
We had to make a redirect/rewrite rule in apache, but the page load is not pretty 😖
Any milestones for this fix yet?

@rubberduck203
Copy link

@marcelofpfelix is there any chance you can share your rewrite rule? I spent a day trying to create one that worked.

@Serubin
Copy link
Contributor
Serubin commented Mar 22, 2017

@rubberduck203 @marcelofpfelix Unfortunately because wekan is asynchronous the rewrite rule won't work. The urls are interpreted on the client and not the server so it will never get passed through apache or nginx to allow for a rewrite to work.

@marcelofpfelix
Copy link

You're right @Serubin. My workaround was to put the ROOT_URL in http. Since I have a Redirect rule from http to https in apache, the url pass through apache and the rewrite is loaded.

But this is also not working for me, since the page reload takes time and and sidebar is set to hidden xD

@Serubin
Copy link
Contributor
Serubin commented Mar 22, 2017

@Marabon Could you help me out and provide a step by step of how you added my fix to the project? I'm struggling to meteor to recognize my local package.

@Marabon
Copy link
Marabon commented Mar 22, 2017

@Serubin I created a dir in my home dir named localpackages where I cloned kadirahq:flow-router to. I then edited the file for your fix. (I'm guessing the same would work for your package, but as previously mentioned the project was looking for kadira so had to name the local package that)

I had to set an environment variable in my .bashrc and .profile to make meteor aware of the local packages. (At first it didn't work when I tried to add the variable but that's because meteor looks for a different variable after a certain version, and we're using a version before that change)

export PACKAGE_DIRS="$HOME/localpackages"

I also made the version of the local package one version higher than the original one to make sure that it found and used the right one.
I used these commands to remove and add the package to the project (It should say which version it updates to):
meteor remove kadira:flow-router
meteor add kadira:flow-router

Hope this helps.

@Serubin
Copy link
Contributor
Serubin commented Mar 23, 2017

@Marabon Perfect. Thank you

@xet7 xet7 closed this as completed in 2031d10 Mar 24, 2017
@xet7 xet7 added this to the v0.17 milestone Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants
0