Fix to get_schema(s) to use URL prefixed with slash for vapp. #283
Conversation
dmichaels-harvard
commented
Sep 8, 2023
•
dmichaels-harvard
commented
- For the vapp (portal_vapp) case, the URL for ff_utils.get_schema(s) needs to be prefixed with a slash; maybe true in general but nervous about changing that now for fear of wider breakage.
- Also, the response from vapp.get returns a webtest.response.TestResponse, which has a json property which is an object rather than a function; handling this in ff_utils.get_response_json.
| @@ -1022,7 +1022,7 @@ def get_schemas(key=None, ff_env: Optional[str] = None, *, allow_abstract: bool | |||
| base_url = 'profiles/' | |||
| add_on = 'frame=raw' | |||
| if portal_vapp: | |||
| full_url = f"{base_url}?{add_on}" | |||
| full_url = f"/{base_url}?{add_on}" | |||
There was a problem hiding this comment.
i guess this is most conservative. if / is ok in get_metadata, then maybe put the / in the initialization of base_url. But either is OK and this is safest, I guess.
| res_json = res.json() | ||
| if isinstance(res, VirtualAppResponse): | ||
| res_json = res.json | ||
| else: |
There was a problem hiding this comment.
Again this is most conservative but I wonder if this should be type-checking, too. I guess let's go with this for now as most minimal change.
There was a problem hiding this comment.
You mean type checking the json property?
There was a problem hiding this comment.
Oh, I was just wondering if we should do a plain else or another if with another type. But it previously just did .json() with no test, so this will be most compatible. I wouldn't have designed fresh code that way if I knew multiple types were in play. It preferences one default over another when you really don't konw. The presence of the try: around it is an apology for just taking a guess and hopes the damage won't be too great if it guesses wrong.
Pull Request Test Coverage Report for Build 6125878108
💛 - Coveralls |
