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

[Fixes #4322] SESSION_EXPIRED_CONTROL_ENABLE=True breaks GeoNode #4343

Merged
merged 12 commits into from
Apr 9, 2019

Conversation

afabiani
Copy link
Member

@afabiani afabiani commented Apr 4, 2019

No description provided.

@afabiani afabiani self-assigned this Apr 4, 2019
@afabiani afabiani requested review from frafra and t-book April 4, 2019 10:50
@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

Merging #4343 into master will decrease coverage by 0.25%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4343      +/-   ##
==========================================
- Coverage   57.98%   57.72%   -0.26%     
==========================================
  Files         202      203       +1     
  Lines       11718    11838     +120     
  Branches     1683     1713      +30     
==========================================
+ Hits         6795     6834      +39     
- Misses       4375     4454      +79     
- Partials      548      550       +2

@afabiani
Copy link
Member Author

afabiani commented Apr 5, 2019

@frafra seems like spc_geonode does not like the session middleware at all :|

@frafra
Copy link
Contributor

frafra commented Apr 5, 2019

@afabiani tests seem to work only if you wait for a while before running them, maybe because of the 60 seconds timeout you added.

A similar issue happens with the standard Docker setup, and should be avoided in my opinion, or at least we should have a simple and robust mechanism to determine if the system is ready to be used or not. I would prefer to avoid API-only solutions, as even a user should be aware if the platform is ready or not, and if you have your system running with a nice homepage the user will think that everything is fine, while it is not.

@afabiani
Copy link
Member Author

afabiani commented Apr 5, 2019

@frafra agree, any suggestion? I'll start thinking about a solution too as soon as I can devote some time here again.

@afabiani
Copy link
Member Author

afabiani commented Apr 5, 2019

@frafra ... which timeout? The DB one?

@afabiani
Copy link
Member Author

afabiani commented Apr 5, 2019

@frafra what about a geonode endpoint http://localhost:8000/gs/online returning a json {"online": false} or {"online": false} whenever the LOCAL_GEOSERVER is up or not?

@t-book
Copy link
Contributor

t-book commented Apr 6, 2019

least we should have a simple and robust mechanism to determine if the system is ready to be used or not.

@frafra what about a geonode endpoint http://localhost:8000/gs/online returning a json {"online": false} or {"online": false} whenever the LOCAL_GEOSERVER is up or not?

I haven´t followed all of the discussion but why would it not be enough checking the status code of geoserver endpoint?

as even a user should be aware if the platform is ready or not,

+1 for informing users whenever gs is down.

fetch('/geoserver/web/').then(function(response) {
  if (response.status !== "200") {
    alert("warning geoserver is offline")
});

@frafra
Copy link
Contributor

frafra commented Apr 8, 2019

@frafra ... which timeout? The DB one?

Yes, but I am not sure if that's what causes the issue.

@frafra what about a geonode endpoint http://localhost:8000/gs/online returning a json {"online": false} or {"online": false} whenever the LOCAL_GEOSERVER is up or not?

In that scenario, how to determine if LOCAL_GEOSERVER is up? Should we check if GeoServer returns a 2xx HTTP code? We are already doing that in geonode-selenium, thanks to Docker healthchecks, but it is still failing on this pull request.

I haven´t followed all of the discussion but why would it not be enough checking the status code of GeoServer endpoint?

That's what is already happening. It seems to me that there are certain cases where GeoNode and GeoServer can be both up and running, but not ready to accept uploads. It would be nice to understand why, because we could fix the standard Docker setup too (which is not possible to test automatically with geonode-selenium because we do not know when it is really ready to be tested #4259).

@afabiani
Copy link
Member Author

afabiani commented Apr 8, 2019

@frafra uhm, I'm not fully sure it is a timeout problem here. It looks to me more a setup issue instead. Something related to the fixtures, not loaded correctly or something. However, I'll investigate a bit more on this as soon as I have some time slots available.

@afabiani
Copy link
Member Author

afabiani commented Apr 8, 2019

@frafra @t-book, for the time being, I will set the property to "False" by default and I will add (in the next commit) appropriate documentation.

I then will open a specific issue about this "spc-geonode" behavior described above.

@afabiani
Copy link
Member Author

afabiani commented Apr 9, 2019

@frafra @t-book with the latest commits:

  1. The Django message framework now uses the cookie store, thus avoiding to lock the DB
  2. The SESSION_EXPIRED_CONTROL_ENABLED is set to False by default and can be enabled to Env
  3. Updated tutorials/advanced/geonode_settings/settings.html variables by describing the new variable setting

image

This PR also refers to #4325 "List of documentation improvements" by refactoring the tutorials/advanced/geonode_settings/settings.txt accordingly.

@t-book
Copy link
Contributor

t-book commented Apr 9, 2019

Great @afabiani. Thanks a lot for this. The blocking does not occur in dev setup anymore and hence the issue is fixed for me.
@frafra This should solve the docker issue as well?

@afabiani
Copy link
Member Author

afabiani commented Apr 9, 2019

I'm going to merge this and address the SPC GeoNode issue here

#4350

@frafra
Copy link
Contributor

frafra commented Apr 9, 2019

@t-book I am not sure about that, but we will see :)

@afabiani afabiani merged commit f62eaf7 into master Apr 9, 2019
@afabiani afabiani deleted the ISSUE_4322 branch April 9, 2019 12:49
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.

3 participants