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 ':root', rather than alias 'root' for default root collection #207

Merged
merged 6 commits into from
Oct 31, 2024

Conversation

ekraffmiller
Copy link
Contributor

@ekraffmiller ekraffmiller commented Oct 28, 2024

What this PR does / why we need it:

The root collection alias, 'root' is not a constant, it is an editable field in the Collection. So we need to use the special keyword ':root' to get the root collection. This affects all use cases that default to the root collection if no collectionId param is provided.

Which issue(s) this PR closes:

Related Dataverse PRs:

  • Depends on #

Special notes for your reviewer:

Suggestions on how to test this:

Visual inspection of the code, including the updated Readme for the use cases, and review of the tests that use the default root collection.

Is there a release notes update needed for this change?:

Additional documentation:

@ekraffmiller ekraffmiller marked this pull request as draft October 28, 2024 14:25
@ekraffmiller ekraffmiller changed the title use special key ':root', rather than alias 'root' to get the root col… Use ':root', rather than alias 'root' to get the root col… Oct 28, 2024
@ekraffmiller ekraffmiller changed the title Use ':root', rather than alias 'root' to get the root col… Use ':root', rather than alias 'root' for default root collection Oct 28, 2024
@ekraffmiller ekraffmiller marked this pull request as ready for review October 29, 2024 13:13
@ekraffmiller ekraffmiller added GREI Re-arch GREI re-architecture-related FY25 Sprint 9 FY25 Sprint 9 (2024-10-23 - 2024-11-06) Size: 3 A percentage of a sprint. 2.1 hours. Original size: 3 SPA.Q4 Not related to any specific Q4 feature SPA.Q4.10 Resolve TODOs and tech debt labels Oct 29, 2024
@g-saracca g-saracca self-assigned this Oct 30, 2024
Copy link
Contributor

@g-saracca g-saracca left a comment

Choose a reason for hiding this comment

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

This looks really good @ekraffmiller!
I suggested a commit (maybe is not needed) and have a doubt about this part of the code in collectionHelper.ts, should be use ':root' instead of 'root' also there?

export async function createCollectionViaApi(
  collectionAlias: string,
  parentCollectionAlias: string | undefined = undefined
): Promise<CollectionPayload> {
  try {
    if (parentCollectionAlias == undefined) {
      parentCollectionAlias = 'root'
    }

Another thing, just out of curiosity, do all API endpoints work with this special id ':root'?

src/collections/domain/useCases/GetCollection.ts Outdated Show resolved Hide resolved
@ekraffmiller
Copy link
Contributor Author

Another thing, just out of curiosity, do all API endpoints work with this special id ':root'?

This looks really good @ekraffmiller! I suggested a commit (maybe is not needed) and have a doubt about this part of the code in collectionHelper.ts, should be use ':root' instead of 'root' also there?

export async function createCollectionViaApi(
  collectionAlias: string,
  parentCollectionAlias: string | undefined = undefined
): Promise<CollectionPayload> {
  try {
    if (parentCollectionAlias == undefined) {
      parentCollectionAlias = 'root'
    }

Another thing, just out of curiosity, do all API endpoints work with this special id ':root'?

Yes, it should be :root thanks!
All of the endpoints should work with this id, as far as I know. I left two use cases as is (not using :root). For 'publishCollection', the root collection should always be published, so I didn't think it needed it. For getCollectionItems, the API call doesn't require collectionId.

@ekraffmiller ekraffmiller removed their assignment Oct 30, 2024
fix comments in GetCollection.ts

Co-authored-by: German Gonzalo Saracca <[email protected]>
@g-saracca g-saracca removed their assignment Oct 31, 2024
@ofahimIQSS ofahimIQSS self-assigned this Oct 31, 2024
@ofahimIQSS
Copy link
Contributor

Did visual inspection of code. Merging PR

@ofahimIQSS ofahimIQSS merged commit a40c600 into develop Oct 31, 2024
5 checks passed
@ofahimIQSS ofahimIQSS deleted the 191-dynamic-root-alias branch October 31, 2024 15:21
@ofahimIQSS ofahimIQSS removed their assignment Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FY25 Sprint 9 FY25 Sprint 9 (2024-10-23 - 2024-11-06) GREI Re-arch GREI re-architecture-related Original size: 3 Size: 3 A percentage of a sprint. 2.1 hours. SPA.Q4 Not related to any specific Q4 feature SPA.Q4.10 Resolve TODOs and tech debt
Projects
Status: Done 🧹
Development

Successfully merging this pull request may close these issues.

Support dynamic alias for the root collection
3 participants