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

Rename submission.dac to submission.rems #648

Merged
merged 1 commit into from
Nov 30, 2022

Conversation

csc-jm
Copy link
Collaborator

@csc-jm csc-jm commented Nov 16, 2022

Description

Separated this task from the other tasks given in #588 for less chaotic reviewing. So this just refactors the naming scheme of the dac field in the submission object to rems.

Related issues

Partially fixes #588

Type of change

  • Breaking change (probably for front-end, considering the endpoint name is altered)

Changes Made

  • change the submission schema
  • change the PUT endpoint name to /v1/submission/rems
  • modify existing integration tests
  • edit openapi specs

Testing

  • Integration Tests

@csc-jm csc-jm force-pushed the refactor/rename-submission-dac branch from f48a0c6 to e6b33ee Compare November 16, 2022 13:43
@csc-jm csc-jm self-assigned this Nov 16, 2022
@csc-jm csc-jm force-pushed the refactor/rename-submission-dac branch from e6b33ee to 6c2383b Compare November 16, 2022 13:55
@csc-jm csc-jm marked this pull request as ready for review November 16, 2022 13:56
@blankdots blankdots requested a review from csc-felipe November 22, 2022 15:13
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

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

looks good to me, but i think we should keep terminology consistent with the source, REMS has workflow and approvers/Handlers make up a DAC

docs/specification.yml Show resolved Hide resolved
Copy link
Contributor

@csc-felipe csc-felipe left a comment

Choose a reason for hiding this comment

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

Looks good to me. Well done, finding all the occurrences to be renamed.

After Stefan's comments are resolved, please dismiss the review and request it again. ^^

@csc-jm csc-jm force-pushed the refactor/rename-submission-dac branch from 6c2383b to c4a931f Compare November 22, 2022 16:32
@csc-jm
Copy link
Collaborator Author

csc-jm commented Nov 22, 2022

While converting all these DAC namings, I also looked at this but wasn't sure if that dataset key needs to change or stay like that.

@blankdots
Copy link
Contributor

While converting all these DAC namings, I also looked at this but wasn't sure if that dataset key needs to change or stay like that.

🤔 what is the dataset key in the lines you highlighted, could we get more details ?

@csc-felipe
Copy link
Contributor

csc-felipe commented Nov 23, 2022

That's adding the REMS details to a dataset. For each dataset we create a resource, and a catalogue item in REMS. We don't use that information anywhere at the moment, but still, it should be preserved. What line 422 does is that it adds the REMS link that can be used to apply for access to the dataset to the dataset description that will be visible in Etsin.

My answer to @csc-jm would be that dac could be renamed to rems also in that case. I expect that later the frontend will display some information derived from these values.

@csc-felipe
Copy link
Contributor

I do think that writing REMS DAC makes sense, as what we're doing is translating REMS data into DAC. But I don't mind if you prefer to have different wording. The most important imo is that it's consistent, and at least somewhat accurate.

@csc-felipe
Copy link
Contributor

On the other hand, writing some documentation how the publishing process works would be good. I just wonder if now is a good time, or after we make the changes we've been planning.

@blankdots
Copy link
Contributor

blankdots commented Nov 23, 2022

That's adding the REMS details to a dataset. For each dataset we create a resource, and a catalogue item in REMS. We don't use that information anywhere at the moment, but still, it should be preserved. What line 422 does is that it adds the REMS link that can be used to apply for access to the dataset to the dataset description that will be visible in Etsin.

hmm i see, so it is added to the: https://github.com/CSCfi/metadata-submitter/blob/develop/metadata_backend/helpers/schemas/ena_dataset.json or https://github.com/CSCfi/metadata-submitter/blob/develop/metadata_backend/helpers/schemas/ena_bpdataset.json this info - but we also have this information in https://github.com/CSCfi/metadata-submitter/blob/refactor/rename-submission-dac/metadata_backend/helpers/schemas/submission.json#L1165 is that correct ?

My question would be what do we do with a submission that has multiple datasets each with their own worflow in REMS because a submission can have multiple datasets (which correspond to a REMS resource) and for each one in rems you need to create a separate catalogue item, however https://github.com/CSCfi/metadata-submitter/blob/refactor/rename-submission-dac/metadata_backend/helpers/schemas/submission.json#L1165 allows for only one object not an array ?

Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

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

On the other hand, writing some documentation how the publishing process works would be good. I just wonder if now is a good time, or after we make the changes we've been planning.

yeah, i think this needs to come with some documentation on what represents a REMS DAC and how we map concepts, otherwise this will not go well with further development

@csc-felipe
Copy link
Contributor

but we also have this information in

@blankdots, not really. Most of the information should be the same, but there are values which are specific to the dataset: resource_id and catalogue_item_id. Also the URL which is build from those values.

@csc-felipe
Copy link
Contributor

csc-felipe commented Nov 23, 2022

As for the second question, in a meeting with other people, we decided that a submission has the same workflow / policy / dac for all datasets. That's why it's an object in the submission. Mostly so that the frontend and backend "remembers" what was the chosen one, and it doesn't have to be specified for each dataset.

But then we do need to create new records in REMS for each dataset, so that data ends up in the metadata_object.

@blankdots
Copy link
Contributor

but we also have this information in

@blankdots, not really. Most of the information should be the same, but there are values which are specific to the dataset: resource_id and catalogue_item_id. Also the URL which is build from those values.

then we need to remove these lines:

@csc-felipe
Copy link
Contributor

Indeed, there was a bug. The issue was created :) #649

@csc-jm csc-jm changed the title Rename submission.dac to submission.rems WIP: Rename submission.dac to submission.rems Nov 29, 2022
@csc-jm csc-jm force-pushed the refactor/rename-submission-dac branch from a44f5a4 to f0fa684 Compare November 29, 2022 16:10
@csc-jm csc-jm force-pushed the refactor/rename-submission-dac branch from f0fa684 to 7185408 Compare November 29, 2022 16:27
@csc-jm csc-jm changed the title WIP: Rename submission.dac to submission.rems Rename submission.dac to submission.rems Nov 29, 2022
@csc-jm
Copy link
Collaborator Author

csc-jm commented Nov 29, 2022

Moved this momentarily to WIP because I need to push and test some things. Sorry if it still generated bunch unnecessary emails. I renamed the dac field of a dataset. I almost also fixed the issue #649 opened by @csc-felipe in this PR but wanted to clarify something first so didn't. Before this PR gets more confusing to review, we can maybe merge and follow up with more issues

Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

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

i am ok with changes so far.

@csc-felipe csc-felipe merged commit 8163946 into develop Nov 30, 2022
@csc-felipe csc-felipe deleted the refactor/rename-submission-dac branch November 30, 2022 08:29
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.

3 participants