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

Models and Resolvers for License, MetadataStandard, Repository and ResearchDomain #169

Merged
merged 11 commits into from
Jan 22, 2025

Conversation

briri
Copy link
Collaborator

@briri briri commented Jan 10, 2025

Description

Fixes #121

Models and resolvers for Licenses, MetadataStandards, Repositories and ResearchDomains. These objects will be used to support the Project and Plan pages.

Added the models for ProjectContributor and ProjectFunder.

I also fixed a typo I found in the data migration that adds bestPractice to the Section table

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Include any relevant details for your test configuration.

I added a lot of unit tests for each new model and also made sure the Queries were working in the Apollo explorer. I did not have a chance to thoroughly verify all of the Mutations though.

Please note there are some new ENV variables that will need to be added to your local .env file!

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I updated the CHANGELOG.md and added documentation if necessary
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@briri briri requested a review from jupiter007 January 10, 2025 11:25
@@ -4,6 +4,7 @@ import { verifyCriticalEnvVariable } from "../utils/helpers";
verifyCriticalEnvVariable('DOMAIN');
verifyCriticalEnvVariable('APP_NAME');
verifyCriticalEnvVariable('DEFAULT_AFFILIATION_URI');
verifyCriticalEnvVariable('DMP_ID_SHOULDER');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would we want to add DMP_ID_BASE_URL or the orcid or ror base urls to this verification list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have default values in place for those. The shoulder though is tied to a specific organization (e.g. DMP Tool has one, Dryad has another, etc.), so I decided not to provide a default for it.

@@ -52,7 +52,7 @@ export const resolvers: Resolvers = {
}

const affiliation = new Affiliation(input);
return await affiliation.create(context);
return await affiliation.update(context);
Copy link
Collaborator

@jupiter007 jupiter007 Jan 14, 2025

Choose a reason for hiding this comment

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

Can you call update here on an affiliation that does not yet exist? It looks like on L50, that we throw an error if it's an existing affiliation, so I guess there would be no existing affiliation when we hit L55.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, we do a check for existence before it, so it shouldn't get to line 55. If it does though a MySQL error should bubble up, so I think it's ok

Copy link
Collaborator

Choose a reason for hiding this comment

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

@briri can you do a sanity check? I just tried to run updateAffiliation and got an error because no id is available in the object. PReviously, when we were using .create() we didn't need an affiliation id.

},

Mutation: {
// add a new ContributorRole
Copy link
Collaborator

Choose a reason for hiding this comment

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

L23: Do you mean add a new addMetadataStandard?

},

Mutation: {
// add a new ContributorRole
Copy link
Collaborator

Choose a reason for hiding this comment

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

L31: Same here. I think you meant add a new repository

@jupiter007
Copy link
Collaborator

Brian, your changes look good. I just had a few comments.

Base automatically changed from feature/BR-project-schema to development January 21, 2025 15:44
@briri
Copy link
Collaborator Author

briri commented Jan 21, 2025

Updated the 2 comments you pointed out above @jupiter007 and also rebased with the latest in development

@briri
Copy link
Collaborator Author

briri commented Jan 22, 2025

@jupiter007 I just rebased this against development (your changes for the versionedTemplates). This one was approved a couple of weeks ago, please reapprove when you have a chance

@briri
Copy link
Collaborator Author

briri commented Jan 22, 2025

ok @jupiter007 the updateAffiliation should now be working

@jupiter007
Copy link
Collaborator

ok @jupiter007 the updateAffiliation should now be working

Thanks @briri

@briri briri merged commit b9b0b8e into development Jan 22, 2025
@briri briri deleted the feature/BR-proj-lookups branch January 22, 2025 19:22
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