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

Resolved infinite loop between two layering PDPs, added new class to make resolving this easier #944

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

tjcouch-sil
Copy link
Member

@tjcouch-sil tjcouch-sil commented Jun 18, 2024

[Breaking] Had to add a new requirement for layering PDPFs. Just extend from the class LayeringProjectDataProviderEngineFactory available on @papi/backend in order to fulfill this requirement automatically.

  • Long explanation of the requirement: getAvailableProjects now receives a parameter which is some subset of the filtering options being used while calling getAvailableProjects on this PDPF. The subset of filtering options will primarily contain some set of PDPF ids to exclude from the search in order to avoid infinitely recurring calls to other layering PDPFs' getAvailableProjects. Base PDPFs can ignore that parameter, but layering PDPFs that call papi.projectLookup.getMetadataForAllProjects to get some set of existing project ids to layer over absolutely must now merge the filter options passed into getAvailableProjects into the filter options they use when calling papi.projectLookup.getMetadataForAllProjects.

This change is Reviewable

lyonsil
lyonsil previously approved these changes Jun 18, 2024
Copy link
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)


src/shared/models/project-data-provider-engine-factory.model.ts line 27 at r1 (raw file):

 * {@link IBaseProjectDataProvider} and {@link ProjectDataProviderInterfaces} for more information.
 *
 * WARNING: For Layering PDPFs, `getAvailableProjects` has very specific requirements that

It seems a little odd to put a big warning here without explaining the warning in some way. This and the following paragraph make it seem like there is a special handshake that gets kicked out of the club if you don't know it.

Copy link
Member Author

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 12 files reviewed, 1 unresolved discussion (waiting on @lyonsil)


src/shared/models/project-data-provider-engine-factory.model.ts line 27 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

It seems a little odd to put a big warning here without explaining the warning in some way. This and the following paragraph make it seem like there is a special handshake that gets kicked out of the club if you don't know it.

handshake.gif

Does the link make it better?

Copy link
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


src/shared/models/project-data-provider-engine-factory.model.ts line 27 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

handshake.gif

Does the link make it better?

LOL - At first nope, but now that I see it in Reviewable I'm all good. 😄

image.png

@tjcouch-sil tjcouch-sil merged commit da79f82 into main Jun 18, 2024
7 checks passed
@tjcouch-sil tjcouch-sil deleted the fix-layering-pdp branch June 18, 2024 20:41
Copy link
Member Author

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


src/shared/models/project-data-provider-engine-factory.model.ts line 27 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

LOL - At first nope, but now that I see it in Reviewable I'm all good. 😄

image.png

Oh sorry I meant the {@link... I added in the TSDoc. Thanks!

This pull request was closed.
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