e-mission / op-admin-dashboard

An admin/deployer dashboard for the NREL OpenPATH platform
0 stars 9 forks source link

Adding '/admin/' to default URL #23

Closed swastis10 closed 1 year ago

swastis10 commented 1 year ago

When we are deploy the app to staging, it is being served from the /admin URL therefore we need to add url_base_pathname. Solution link : https://community.plotly.com/t/mod-wsgi-and-dash-virtual-host-configuration/28687

Testing done:

  1. Cognito works fine
  2. http://localhost:8050/admin/ is loading
  3. Other URLs such as http://localhost:8050/admin/tokens are also loading fine
shankari commented 1 year ago

@swastis10 that is not actually the solution suggested in the link

The solution (at https://community.plotly.com/t/mod-wsgi-and-dash-virtual-host-configuration/28687/2) is to specify it once while setting up the dash app app = dash.Dash(requests_pathname_prefix='/myapp/') instead of putting it into every single URL path.

Is there a reason why you did not use that approach?

Also, you have indicated that the /admin path prefix works. But have you tested with other path prefixes? In particular, does it still work with no path prefix environment variable (using the default prefix of /)?

Finally, can you please ensure that we don't introduce extraneous whitespace changes. Look at the diffs for this PR - it is almost impossible to see what the real changes are since almost every line has supposedly changed.

This will also mess up the blame and make it harder to work with the code later.

swastis10 commented 1 year ago

@swastis10 that is not actually the solution suggested in the link

The solution (at https://community.plotly.com/t/mod-wsgi-and-dash-virtual-host-configuration/28687/2) is to specify it once while setting up the dash app app = dash.Dash(requests_pathname_prefix='/myapp/') instead of putting it into every single URL path.

Is there a reason why you did not use that approach?

Also, you have indicated that the /admin path prefix works. But have you tested with other path prefixes? In particular, does it still work with no path prefix environment variable (using the default prefix of /)?

Finally, can you please ensure that we don't introduce extraneous whitespace changes. Look at the diffs for this PR - it is almost impossible to see what the real changes are since almost every line has supposedly changed.

This will also mess up the blame and make it harder to work with the code later.

@shankari, I have used

app = Dash(
    external_stylesheets=[dbc.themes.BOOTSTRAP, dbc.icons.FONT_AWESOME],
    suppress_callback_exceptions=True,
    use_pages=True,
    url_base_pathname=url_path_prefix
)

Which loads the app at http://0.0.0.0:8050/admin/. Now if I go to the sidebar and click on, say, "Map" - It will redirect me towards http://0.0.0.0:8050/map/ which is not correct. Therefore, I have changed every single URL path to accomodate - http://0.0.0.0:8050/admin/map.

If I do not provide any path prefix env variable, the app will load like it used to before i.e using the default prefix of /. URL - http://0.0.0.0:8050/

shankari commented 1 year ago

@swastis10 is there a reason that you are using url_base_pathname instead of the recommended requests_pathname_prefix from the answer? It looks like requests_pathname_prefix might prepend the pathname to all requests by default.

If I do not provide any path prefix env variable, the app will load like it used to before i.e using the default prefix of /. URL - http://0.0.0.0:8050/

And you have tested this? Or you assume it based on the code?

swastis10 commented 1 year ago

requests_pathname_prefix

@shankari url_base_pathname seemed like a more appropriate choice as requests_pathname_prefix did not work. From dash documentation:

url_base_pathname A local URL prefix to use app-wide. Default '/'. Both requests_pathname_prefix and routes_pathname_prefix default to url_base_pathname. env: DASH_URL_BASE_PATHNAME

Also,

If I do not provide any path prefix env variable, the app will load like it used to before i.e using the default prefix of /. URL - http://0.0.0.0:8050/

I have tested this

shankari commented 1 year ago

@swastis10 thank you for the clarification. Just to set expectations, it would have been helpful to state that upfront while creating the PR. Again, think about an intern looking at this code 10 years later - you have linked to a resource that says to use requests_pathname_prefix but you are not using it, wouldn't they want to know why?

And this is still not fully clear. wrt requests_pathname_prefix did not work? What do you mean "did not work"? Did it have the same behavior as url_base_pathname? Because arguably url_base_pathname also did not work until you added the prefix to all the routes. Please look at my issues as an example of how to indicate what worked and did not work

Finally, while I will merge this now given the time sensitivity, I still don't think that this is the correct solution. The correct solution should be to specify the URL base while creating the app. There are other URL paths, which we admittedly don't use a lot of of - css/javascript/images etc. It is not a viable solution to go and change every single URL to have a prefix.

For the long term, you should indicate exactly why requests_pathname_prefix (with or without the default value) did not work and figure out how to get it to work. Notably, try https://dash.plotly.com/reference#dash.get_relative_path

Please file an issue for the cleanup task.

swastis10 commented 1 year ago

Adding the link to url_base_pathname

shankari commented 1 year ago

I am merging this for now given the time sensitivity, but I don't think that it is the long-term solution. @swastis10 please file an issue for the cleanup and I will merge

swastis10 commented 1 year ago

http://0.0.0.0:8050/

@shankari issue #24

shankari commented 1 year ago

@swastis10 merged