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

Generate static urls #11571

Open
wants to merge 20 commits into
base: dev/8.0.x
Choose a base branch
from
Open

Conversation

chrabyrd
Copy link
Contributor

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

This removes dependency on arches_urls.htm for all Vue frontend work.

Issues Solved

Closes #11567

Checklist

  • I targeted one of these branches:
    • dev/8.0.x (under development): features, bugfixes not covered below
    • dev/7.6.x (main support): regressions, crashing bugs, security issues, major bugs in new features
    • dev/6.2.x (extended support): major security issues, data loss issues
  • I added a changelog in arches/releases
  • I submitted a PR to arches-docs (if appropriate)
  • Unit tests pass locally with my changes
  • I added tests that prove my fix is effective or that my feature works
  • My test fails on the target branch

Accessibility Checklist

Developer Guide

Topic Changed Retested
Color contrast
Form fields
Headings
Links
Keyboard
Responsive Design
HTML validation
Screen reader

Ticket Background

  • Sponsored by:
  • Found by: @
  • Tested by: @
  • Designed by: @

Further comments

@chrabyrd chrabyrd requested a review from aarongundel October 28, 2024 17:01
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Principle makes sense. Noticed one behavior change I think we should look into. Will kick the tires again after.

arches/app/templates/base-root.htm Show resolved Hide resolved
arches/app/templates/base.htm Show resolved Hide resolved
arches/app/utils/frontend_configuration_utils.py Outdated Show resolved Hide resolved
releases/8.0.0.md Outdated Show resolved Hide resolved
releases/8.0.0.md Show resolved Hide resolved
releases/8.0.0.md Outdated Show resolved Hide resolved
arches/app/utils/frontend_configuration_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

I cross-checked this against the new listurls feature likely to land in Django 5.2 and noticed that by keying the dict on name, we're overwriting and losing additional routes that lack a name, e.g. tileserver/<path>.

re_path(r"^tileserver/(?P<path>.*)$", TileserverProxyView.as_view()),

Maybe we can have an additional data structure to hold unnamed routes?

@robgaston robgaston requested review from chiatt and removed request for robgaston November 11, 2024 19:02
@robgaston robgaston removed their assignment Nov 11, 2024
Copy link
Member

@chiatt chiatt left a comment

Choose a reason for hiding this comment

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

Can arches_urls.htm be removed from the project template?


1. Then update your project:
```
python manage.py updateproject
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to let people know that they can delete their arches_urls.htm file (I'd suggest using update project, but that might be a little intrusive)

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Hey, just circling back here. I think the issue I mentioned above about unnamed routes is not so serious, given that we weren't using them on the front end before.

But there's a similar issue with routes we do use on the frontend like plugins.

There are four routes named "plugins". This is fine, since reverse() matches on the number and names of args:

arches/arches/urls.py

Lines 652 to 655 in f3390ef

path("plugins/<uuid:pluginid>", PluginView.as_view(), name="plugins"),
path("plugins/<uuid:pluginid>/<path:path>", PluginView.as_view(), name="plugins"),
path("plugins/<slug:slug>", PluginView.as_view(), name="plugins"),
path("plugins/<slug:slug>/<path:path>", PluginView.as_view(), name="plugins"),

But since we're keying on name, we can hold at most one. The one that wins out is:

    "plugins": "/plugins/{slug}/{path}",

That's the form we happen to be using in the controlled list manager, e.g. /plugins/controlled-list-manager/item/pk

However, the bulk data manager uses
/plugins/bulk-data-manager,

And I don't think a client could generate the URL for that, if this test case is right:

diff --git a/arches/app/src/arches/utils/generate-arches-url.test.ts b/arches/app/src/arches/utils/generate-arches-url.test.ts
index 68fab4bf88..d7f752450f 100644
--- a/arches/app/src/arches/utils/generate-arches-url.test.ts
+++ b/arches/app/src/arches/utils/generate-arches-url.test.ts
@@ -7,9 +7,14 @@ global.ARCHES_URLS = {
     another_url: "/admin/another/{id}",
     multi_interpolation_url:
         "/{language_code}/resource/{resource_id}/edit/{field_id}/version/{version_id}",
+    plugins: "/plugins/{slug}/{path}",
 };
 
 describe("generateArchesURL", () => {
+    it("should return simple plugin url", () => {
+        const result = generateArchesURL("plugins", { slug: "bulk-data-manager" });
+        expect(result).toBe("/plugins/bulk-data-manager");
+    });
     it("should return a valid URL with specified language code and parameters", () => {
         const result = generateArchesURL("example_url", { id: "123" }, "fr");
         expect(result).toBe("/fr/admin/example/123");
 FAIL  arches/app/src/arches/utils/generate-arches-url.test.ts > generateArchesURL > should return simple plugin url
AssertionError: expected '/plugins/bulk-data-manager/{path}' to be '/plugins/bulk-data-manager' // Object.is equality

- Expected
+ Received

- /plugins/bulk-data-manager
+ /plugins/bulk-data-manager/{path}

Do you see the same?

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.

Generate static urls for frontend consumption
4 participants