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

Use NextJS catch-all segments instead of URN #5396

Merged
merged 2 commits into from
Oct 18, 2024

Conversation

gilluminate
Copy link
Contributor

@gilluminate gilluminate commented Oct 17, 2024

Closes #

Description Of Changes

Utilize NextJS catch-all segments to avoid the need for "." separated subfieldURN and encode/decode all field names when used as URL parts.

(see https://nextjs.org/docs/pages/building-your-application/routing/dynamic-routes#catch-all-segments)

image

Code Changes

  • Implement catch-all segment route
  • Replace URN usage with new catch-all segment support instead
  • Include styles to allow breadcrumbs to get super long and scrollable
  • Hides Taxonomy Picker on rows with subfields until that bug gets fixed

Steps to Confirm

  • Add the following dataset
- fides_key: example_dataset_issue_hj36
  organization_fides_key: default_organization
  tags: null
  name: example_dataset_issue_hj36
  description: Minimal dataset reproducing issue (HJ-36)
  meta: null
  data_categories: null
  fides_meta: null
  collections:
    - name: example_table
      description: Example description
      data_categories: []
      fields:
        - name: example_nested_field
          description: null
          data_categories: null
          fides_meta: null
          fields:
            - name: example_success_nested_field
              description: Can save data category changes on nested field
              data_categories:
                - user.device
              fides_meta: null
              fields: []
            - name: example_failure_nested_field.1
              description: >-
                Fail to save data category changes on nested field with '.' in
                name
              data_categories: []
              fides_meta: null
              fields:
                - name: some.thing/Stupid-that's_redicuLous&
                  description: tesing some random string
                  data_categories: []
                  fides_meta: null
                  fields:
                    - name: another.dumb:th!ng
                      description: tesing some random string
                      data_categories:
                        - user.location
                      fides_meta: null
                      fields: null
        - name: example_success_field
          description: Can save data category changes
          data_categories:
            - user.device
          fides_meta: null
          fields: null
        - name: example_success_field.1
          description: Can save data category changes
          data_categories:
            - user.device
          fides_meta: null
          fields: null
      fides_meta: null
  • Navigate down the example_dataset_issue_hj36 table as long as there are subfields available
  • Even with crazy names, the URLs and breadcrumbs should behave as expected
  • At the end where "another.dumb:th!ng" row exists, make sure you can add a new Data Category

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Copy link

vercel bot commented Oct 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Oct 17, 2024 11:32pm

Copy link

cypress bot commented Oct 17, 2024

fides    Run #10508

Run Properties:  status check passed Passed #10508  •  git commit 7147d0497d ℹ️: Merge 552ad78436d973cf2357a4f688a917ee95815e44 into cb0579859b31e8f3b8c8ef5c232f...
Project fides
Run status status check passed Passed #10508
Run duration 00m 37s
Commit git commit 7147d0497d ℹ️: Merge 552ad78436d973cf2357a4f688a917ee95815e44 into cb0579859b31e8f3b8c8ef5c232f...
Committer Jason Gill
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Copy link
Contributor

@lucanovera lucanovera left a comment

Choose a reason for hiding this comment

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

Great work 💯 . This is a better pattern, it's great that is more robust in handling weird characters and simpler because of the delegation of parsing the url to next.js. I tested this and other datasets without any problems.

@gilluminate gilluminate merged commit fa1b922 into main Oct 18, 2024
13 checks passed
@gilluminate gilluminate deleted the HJ-36-dataset-with-nested-fields-2 branch October 18, 2024 16:55
Copy link

cypress bot commented Oct 18, 2024

fides    Run #10513

Run Properties:  status check passed Passed #10513  •  git commit fa1b922c52: Use NextJS catch-all segments instead of URN (#5396)
Project fides
Run status status check passed Passed #10513
Run duration 00m 39s
Commit git commit fa1b922c52: Use NextJS catch-all segments instead of URN (#5396)
Committer Jason Gill
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

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.

2 participants