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

Handle shortlinks in print #477

Merged
merged 8 commits into from
Jun 5, 2015
Merged

Handle shortlinks in print #477

merged 8 commits into from
Jun 5, 2015

Conversation

elemoine
Copy link
Contributor

This PR factors out the "interact with shorturl web service" code in a specific Angular service, and make the shorturl and the print directives rely on that service.

Unfortunately the "shorturl" web service does not work locally on my dev environment. It returns short URLs that look like this: http://localhost/elemoine/s/PIR4 (the port number has been removed, and the instanceid has been added).

@petzlux, could you please review this? In the mean time, I can try to test it on our dev server.

Fixes #470.

@petzlux
Copy link
Contributor

petzlux commented May 20, 2015

Will do

Concerning the URLs, it seems that c2cgeoportal doesnt support Port number in the template, I don't know about the instance name, which didnt get attached in my setup ... I believe this is an upstream issue in c2cgeoportal

@elemoine
Copy link
Contributor Author

Concerning the URLs, it seems that c2cgeoportal doesnt support Port number in the template, I don't know about the instance name, which didnt get attached in my setup ... I believe this is an upstream issue in c2cgeoportal

Definitely, and one that may not be easy to fix. We will have to talk to @sbrunner about that.

@petzlux
Copy link
Contributor

petzlux commented May 20, 2015

In practice, for our deployment might not matter much, as we will use http://g-o.lu domain , with no port number ...

@petzlux
Copy link
Contributor

petzlux commented May 20, 2015

Shorturl refactor is okay, regarding print functionality, I dont have a working print setup , so cant review this bit. @jaykayone can you help me there ?

@jaykayone
Copy link
Contributor

no working setup here either ..

@jaykayone
Copy link
Contributor

On http://devv3.geoportail.lu/main there is a print service running (tomcat), but the printproxy does not seem to work:

http://devv3.geoportail.lu/main/wsgi/printproxy/status/undefined.json

The page keeps sending bad requests once the print button has been pushed
screen shot 2015-05-21 at 08 55 16

@elemoine
Copy link
Contributor Author

@jaykayone I think it'd be good to discuss that issue elsewhere as it is unrelated to the shorcurl support. I'll create a separate issue.

@jaykayone
Copy link
Contributor

ok

@petzlux
Copy link
Contributor

petzlux commented May 22, 2015

For me looks okay to merge ! @elemoine

@petzlux
Copy link
Contributor

petzlux commented May 22, 2015

Can this be merged and the print problem be adressed in a separate pull request ?

@petzlux
Copy link
Contributor

petzlux commented May 28, 2015

Just wanted to ask again if this can be merged, as I would like to use the shorturl service for #205

@elemoine
Copy link
Contributor Author

I have a problem with merging this because, because of the shortlink issue, printing would not work on people's development environments. So I'd like to find a solution first. @sbrunner suggested a solution to me, but I still need to test it.

Can't you just extract out the shorturl service from my branch into yours? I can rebase my branch onto master if your branch is merged into master before mine.

@petzlux
Copy link
Contributor

petzlux commented May 28, 2015

Okay will try !

@elemoine
Copy link
Contributor Author

elemoine commented Jun 4, 2015

I've made progress with this, but I have one remaining problem: geoportailv3's qr service won't create a QR code image for me because the input URL (which looks like this http://localhost:5000/short/q9R1) does not pass the regex. @jaykayone, do you think we can relax the regex a bit?

@jaykayone
Copy link
Contributor

@petzlux already did that in his right click PR

@elemoine
Copy link
Contributor Author

elemoine commented Jun 5, 2015

@petzlux already did that in his right click PR

Nice!

@elemoine
Copy link
Contributor Author

elemoine commented Jun 5, 2015

I've cherry-picked @petzlux's change to qr.py: df4e4af.

This is ready to be merged from my point of view. @petzlux will need to rebase his branch onto master if we merge my branch before his.

@elemoine
Copy link
Contributor Author

elemoine commented Jun 5, 2015

I am happy to merge this if @petzlux agrees to rebase his branch onto master when this is merged :-)

@petzlux
Copy link
Contributor

petzlux commented Jun 5, 2015

👍 yes will rebase

elemoine pushed a commit that referenced this pull request Jun 5, 2015
@elemoine elemoine merged commit 8287b99 into master Jun 5, 2015
@elemoine elemoine deleted the elemoine_print-shorturl branch June 5, 2015 13:14
@sbrunner sbrunner modified the milestone: 1 Jun 16, 2015
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.

Handle shortlinks in print
4 participants