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 #344] Real Cause of 'rest_framework namespace is not Unique' is not Fixed #345

Merged
merged 3 commits into from
Jul 21, 2021

Conversation

asgharsir
Copy link

@asgharsir asgharsir commented Jul 19, 2021

It clears this:
image

It also fixes duplicate URLs, as we see for mapstore/rest/.
Before it was:
image

Now it is:
image

@giohappy giohappy requested review from afabiani and giohappy July 20, 2021 12:32
Copy link

@giohappy giohappy left a comment

Choose a reason for hiding this comment

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

@asgharsir please fix the issue on master branch and then assigned the backport 3.3.x and backport 3.2.x labels to the PR

@@ -19,5 +19,6 @@

urlpatterns = [
url(r'^rest/', include(router.urls)),
url(r'^api-auth/', include('rest_framework.urls', namespace='rest_framework'))
# rest_framework.urls may be skipped, if it is supposed to be always inside geonode
url(r'^api-auth/', include('rest_framework.urls', namespace='mapstore2_adapter_apis'))
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this one, GeoNode already includes it.

@asgharsir
Copy link
Author

@asgharsir please fix the issue on master branch and then assigned the backport 3.3.x and backport 3.2.x labels to the PR

But @giohappy both has different issues, so different fixes.

Master doesn't have the issue of the unique namespace. It is much refactored and in Django 3.2. The only thing here is it is missing mapstore/rest urls. So, only thing we can do is to add them,

3.2.x which is used in Geonode 3s, that has the unique namespace problem, plus duplicate urls issue.

So now?

@@ -28,5 +29,6 @@ class AppConfig(BaseAppConfig):
label = "geonode_mapstore_client"

def ready(self):
run_setup_hooks()
if not apps.ready:
Copy link
Member

Choose a reason for hiding this comment

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

It looks to me that by adding this check the run_setup_hooks() will never be executed

Copy link
Author

Choose a reason for hiding this comment

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

No Alessio. It still gets executed.

  1. Adds the urls.
  2. Also tested with breakpoints at different places, api/urls.py inclusive.

Copy link
Member

@afabiani afabiani Jul 20, 2021

Choose a reason for hiding this comment

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

Ok, I trust you, haven't tested on my side.

Copy link
Author

Choose a reason for hiding this comment

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

My pleasure. And I had to.

@giohappy
Copy link

@asgharsir please fix the issue on master branch and then assigned the backport 3.3.x and backport 3.2.x labels to the PR

But @giohappy both has different issues, so different fixes.

Master doesn't have the issue of the unique namespace. It is much refactored and in Django 3.2. The only thing here is it is missing mapstore/rest urls. So, only thing we can do is to add them,

3.2.x which is used in Geonode 3s, that has the unique namespace problem, plus duplicate urls issue.

So now?

@asgharsir in that case make the PR target 3.3.x and only add the backport to 3.2.x label.

@asgharsir
Copy link
Author

Created a new PR #349 branching from 3.3.x. I am not sure if I could label it well, only found Backport Needed.

@afabiani afabiani merged commit dec3adf into 3.2.x Jul 21, 2021
@afabiani afabiani deleted the issue-344 branch July 21, 2021 07: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