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

Handle resource url according to new standard #35312

Merged
merged 6 commits into from
Nov 5, 2024
Merged

Conversation

Charl1996
Copy link
Contributor

@Charl1996 Charl1996 commented Nov 1, 2024

Technical Summary

With the merge of this PR it's necessary to update the resource URL to remove the resource name as it's not necessary to specify the resource name manually anymore (@esoergel can expound in more detail if necessary)

Feature flag

Although this change is technically global, only users with the SUPERSET_ANALYTICS feature flag will be directly impacted after this PR has been deployed.

Safety Assurance

Tested locally

Automated test coverage

No QA. Local testing if fine

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@Charl1996 Charl1996 requested review from esoergel and mkangia November 1, 2024 09:59
@dimagimon dimagimon added the Risk: High Change affects files that have been flagged as high risk. label Nov 1, 2024
@Charl1996 Charl1996 added the product/invisible Change has no end-user visible impact label Nov 1, 2024
@@ -1423,5 +1423,5 @@ def prepend_urls(self):
# We're overriding the default "list" view to redirect to "detail" view since
# we already know the user through OAuth.
return [
url(r"^(?P<resource_name>%s)/$" % self._meta.resource_name, self.wrap_view('dispatch_detail')),
url(r"^$", self.wrap_view('dispatch_detail'), name='api_dispatch_detail'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to understand the impact of the linked PR on to this one, but could not really figure it out.

The resource name now comes before the version?
Or may be the question is,
What was the URL earlier and what is it now?

Copy link
Contributor

@esoergel esoergel Nov 1, 2024

Choose a reason for hiding this comment

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

I moved the resource_name out of this portion of the URL and up a level higher.

The conflict between my PR and Charl's meant that (?P<resource_name>%s) would've appeared in the URL twice.

>>> resolve('/a/esoergel/api/v0.5/analytics-roles/analytics-roles/')
ResolverMatch(func=tastypie.resources.wrapper, args=(), kwargs={'domain': 'esoergel', 'api_name': 'v0.5', 'resource_name': 'analytics-roles'},
 url_name=None, app_names=[], namespaces=[], route='^a/(?P<domain>[\\w\\.:-]+)/api/(?P<api_name>v0.5)/(?P<resource_name>analytics-roles)/(?P<r
esource_name>analytics-roles)/$', captured_kwargs={'resource_name': 'analytics-roles'})

Copy link
Contributor

Choose a reason for hiding this comment

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

Thsnks @esoergel

Do users need to update any of the urls they might be using like we do here https://github.com/dimagi/commcare-analytics/blob/master/hq_superset/hq_url.py?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, all the existing URLs should continue to function as before. Though if you guys like, you could make this the first API only available at the new URL (assuming you can be confident that that's the only place referencing this API)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Solid, thank you @Charl1996 @esoergel

@esoergel
Copy link
Contributor

esoergel commented Nov 1, 2024

Oh yeah thank you - I forgot to follow up with this PR. Can you make the resource also available at v1 by putting this at the bottom of urlpatterns in api/urls.py? Or even better yet, if no one is using it at v0.5 yet, you can drop it from there entirely now.

$ git diff
diff --git a/corehq/apps/api/urls.py b/corehq/apps/api/urls.py
index 829ccd2ff5c..0de13afb213 100644
--- a/corehq/apps/api/urls.py
+++ b/corehq/apps/api/urls.py
@@ -101,7 +101,6 @@ _OLD_API_LIST = (
         fixtures.v0_1.LookupTableResource,
         fixtures.v0_1.LookupTableItemResource,
         v0_5.NavigationEventAuditResource,
-        v0_5.CommCareAnalyticsUserResource,
     )),
     ((0, 6), (
         locations.v0_6.LocationResource,
@@ -182,6 +181,7 @@ urlpatterns = [
     fixtures.v0_1.LookupTableItemResource.get_urlpattern('v1'),
     fixtures.v0_6.LookupTableItemResource.get_urlpattern('v2'),
     v0_5.NavigationEventAuditResource.get_urlpattern('v1'),
+    v0_5.CommCareAnalyticsUserResource.get_urlpattern('v1'),
 ]

@esoergel esoergel force-pushed the update-resource-url branch from 4bafa29 to 1e3c1ca Compare November 1, 2024 19:17
@esoergel esoergel force-pushed the update-resource-url branch from 1e3c1ca to 749ad0a Compare November 1, 2024 19:19
@esoergel
Copy link
Contributor

esoergel commented Nov 1, 2024

Reverted #35315 here and made the URL available also in the v1 format

@mkangia
Copy link
Contributor

mkangia commented Nov 3, 2024

FYI
This is now deployed on staging

@@ -182,6 +181,7 @@ def versioned_apis(api_list):
fixtures.v0_1.LookupTableItemResource.get_urlpattern('v1'),
fixtures.v0_6.LookupTableItemResource.get_urlpattern('v2'),
v0_5.NavigationEventAuditResource.get_urlpattern('v1'),
v1_0.CommCareAnalyticsUserResource.get_urlpattern('v1'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh, very nice! Another thing you may consider if it makes resonates with you is to put the resource in the app containing the model it serves. So the users resources in the users app, the groups resource in the groups app, etc. We have a few that operate this way already, and I've been thinking about pushing us to that model eventually, though I certainly haven't established a consensus around it yet.

My thinking is that it's more in line with how Django apps are generally used - one app per "thing", with models, views, tests, forms, fixtures, migrations, etc all in that folder, so related code is close together. Though in this case I'm not certain there's a very natural home for CommCareAnalyticsUserResource - maybe users? Also fine to punt on that question

@Charl1996 Charl1996 merged commit 0be62d4 into master Nov 5, 2024
13 checks passed
@Charl1996 Charl1996 deleted the update-resource-url branch November 5, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact Risk: High Change affects files that have been flagged as high risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants