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

Allow user defined id field in MeasurementPeriods #1274

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

fpotier
Copy link
Member

@fpotier fpotier commented Jun 20, 2024

@fpotier fpotier marked this pull request as ready for review June 20, 2024 16:15
@fpotier fpotier requested a review from nitrosx June 20, 2024 16:16
Copy link
Contributor

@nitrosx nitrosx left a comment

Choose a reason for hiding this comment

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

@fpotier the code looks fine and it passes the test, so if we are in a hurry to get this feature in, I will be happy to approve it.
That said, I would like to propose two different approaches:

  1. place the measurement period list in it's own collection with a proper schema, which will solve the unique id issue.
  2. expand the concept of proposal with proposal type, parent proposal, and metadata (similar to dataset scientificMetadata) which will allow to create a hierarchical structure of proposals entities which has at the root your proposal and under each visit (aka measurement period)

Let's discuss

@fpotier
Copy link
Member Author

fpotier commented Jun 24, 2024

@nitrosx Thanks, no hurry to merge it!
I'm okay with moving MeasurementPeriods to a dedicated collection but I'm not sure that the unique id issue would be solved.
Our user office uses a simple counter for each proposal (1..n), so in our case, we need uniqueness based on the proposal id. There can be an arbitrary number of period with id 1 as long as their proposal ids are different.

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