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

#102 several fixes for (dead) links in navigating HTML #103

Merged
merged 3 commits into from
May 2, 2019

Conversation

justb4
Copy link
Member

@justb4 justb4 commented May 1, 2019

Appears this issue was not really due to #92.
At least no dead links anymore and JSON content when accessed in browser.

Question remains that on single Item the Next and Prev links should really walk through Collection but I suggest to do that in other issue, as this is quite tricky to achieve (as one does not know the prev/next id's in isolation of single Item).

Hold on..one failing test now...

@justb4 justb4 added the bug Something isn't working label May 1, 2019
@justb4 justb4 requested a review from tomkralidis May 1, 2019 16:00
@justb4 justb4 self-assigned this May 1, 2019
@@ -166,3 +166,7 @@ def serve(ctx, debug=False):
# setup_logger(CONFIG['logging'])
APP.run(debug=True, host=api_.config['server']['bind']['host'],
port=api_.config['server']['bind']['port'])


if __name__ == '__main__': # run locally, for testing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? pygeoapi serve accomplishes this already.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main reason for main() here is for (stepwise) debugging within an IDE like IntelliJ IDEA or WebStorm and probably PyCharm: needed easy entry point.

pygeoapi/api.py Outdated
@@ -125,6 +125,10 @@ def root(self, headers, args):
]

if format_ == 'html': # render
for link in fcm['links']:
if 'json' in link['type']:
link['href'] += '?f=json'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this better written as link['href'] = ''.join((link['href'], '?f=json'))? I'm not sure what the current verdict is around 'adding' strings in Python.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I read both + and join can be used, with join being marginally faster with many strings to concatenate. For readablity + is preferred.

@justb4
Copy link
Member Author

justb4 commented May 2, 2019

Checks failed since one test need change in headers() fixture to emulate real HTTP (Werkzeug) Headers structure with embedded environ. Will add. This would also allow for more advanced tests simulating many variations of client requests (think of proxied requests, using SCRIPT_NAME and returning full URLs within response-bodies (i.s.o http://localhost:5000).

@justb4
Copy link
Member Author

justb4 commented May 2, 2019

Also needed to fix the breadcrumbs relative URL paths: in cases where the page URL ends with / the breadcrumbs paths gave stale links.

Now all HTML links are based on Flask/Werkzeug PATH_INFO. This is more robust, as this also would work behind a proxy (when SCRIPT_NAME included).

Also needed to rewrite the test_api.py test to emulate the actual Werkzeug header structure passed in. See also https://werkzeug.palletsprojects.com/en/0.15.x/test/. Incoming request Headers should be immutable, so made distinction between request and response headers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants