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

Alter entity spec version and add branchId, trunkVersion in form schema #1210

Merged
merged 14 commits into from
Oct 16, 2024

Conversation

ktuite
Copy link
Member

@ktuite ktuite commented Oct 3, 2024

Closes getodk/central#692

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

Before submitting this PR, please make sure you have:

  • run make test and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments or that everything is internally sourced

lib/data/schema.js Outdated Show resolved Hide resolved
test/unit/data/schema.js Outdated Show resolved Hide resolved
@ktuite ktuite force-pushed the ktuite/migrateEntitySpecVersion branch 2 times, most recently from a4c6e99 to 3aad326 Compare October 8, 2024 23:34
@ktuite ktuite force-pushed the ktuite/migrateEntitySpecVersion branch from 221371f to 0378d39 Compare October 10, 2024 20:49
@ktuite ktuite marked this pull request as ready for review October 10, 2024 21:12
Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

Just small comments, this is looking good to me. 👍 I haven't taken a look at tests yet, but maybe we could do that interactively.

lib/model/frames/form.js Outdated Show resolved Hide resolved
@@ -27,6 +27,8 @@ const jobs = {
'upgrade.process.form.draft': [ require('./form').updateDraftSet ],
'upgrade.process.form': [ require('./form').updatePublish ],

'upgrade.process.form.entities_version': [ require('./form').updateEntitiesVersion ],
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to account for this new action on Frontend. I'd be happy to do so if that'd be helpful.

lib/worker/form.js Outdated Show resolved Hide resolved
lib/worker/form.js Outdated Show resolved Hide resolved
lib/data/schema.js Outdated Show resolved Hide resolved
lib/worker/form.js Show resolved Hide resolved
lib/worker/form.js Outdated Show resolved Hide resolved
lib/worker/form.js Outdated Show resolved Hide resolved
lib/worker/form.js Outdated Show resolved Hide resolved
Copy link
Member Author

@ktuite ktuite left a comment

Choose a reason for hiding this comment

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

Interactive review with @matthew-white

lib/data/schema.js Show resolved Hide resolved
lib/model/query/forms.js Outdated Show resolved Hide resolved
lib/model/query/forms.js Outdated Show resolved Hide resolved
lib/worker/form.js Outdated Show resolved Hide resolved
test/integration/other/migrations.js Show resolved Hide resolved
test/integration/other/form-entities-version.js Outdated Show resolved Hide resolved
@matthew-white
Copy link
Member

I think I'm realizing that there's a subtle thing happening here with publishedBy, which the API returns on the form def/version. I think the API only returns publishedBy with extended metadata and only from the version list endpoint (…/versions), though I could be wrong about that. But my suspicion is that if the worker job in this PR creates a new published version, then publishedBy will end up being null. In the past, I don't think it's been possible for publishedBy to be null.

I actually don't think that Backend will have any issue with publishedBy being null. For example, the SQL query for publishedBy is already a left join. That said, I think it's probably worth verifying in a test that publishedBy is null for an [upgrade] form def.

I also don't think that Frontend will have an issue with publishedBy being null. It looks like the FormVersionRow component is the only one to reference publishedBy, and it will work fine if publishedBy is null (it just won't show it).

I don't think anything needs to change here except for maybe adding a test. But I wanted to note this as a slight change in thinking about publishedBy, that there's now a concrete case in which it can be missing.

@ktuite
Copy link
Member Author

ktuite commented Oct 16, 2024

I think I'm realizing that there's a subtle thing happening here with publishedBy, which the API returns on the form def/version. I think the API only returns publishedBy with extended metadata and only from the version list endpoint (…/versions), though I could be wrong about that. But my suspicion is that if the worker job in this PR creates a new published version, then publishedBy will end up being null. In the past, I don't think it's been possible for publishedBy to be null.

It looks like

  • publishedBy is not present on the .../forms/xmlFormId request, just createdBy and publishedAt.
  • publishedBy is null for the version published by the worker (only accessible through /versions with extended metadata)
  • It's also null in the audit log (probably why it is null in versions), which seems appropriate
Screenshot 2024-10-16 at 12 16 59 PM

@ktuite ktuite force-pushed the ktuite/migrateEntitySpecVersion branch from 1d9aa33 to c5590af Compare October 16, 2024 19:43
@ktuite ktuite merged commit dcca981 into master Oct 16, 2024
6 checks passed
@ktuite ktuite deleted the ktuite/migrateEntitySpecVersion branch October 16, 2024 20:16
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.

Migrate forms to use offline entities
2 participants