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

fix oversights in add missing schema fields #4736

Merged
merged 1 commit into from
Sep 25, 2024
Merged

fix oversights in add missing schema fields #4736

merged 1 commit into from
Sep 25, 2024

Conversation

boutell
Copy link
Member

@boutell boutell commented Sep 24, 2024

Fix issues that came up in testbed.

@boutell boutell requested a review from haroun September 24, 2024 15:29
Copy link

linear bot commented Sep 24, 2024

const seen = new Set();
const keep = {};
for (const key of Object.keys(changes)) {
// MongoDB won't tolerate $set for both
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like the existing unit tests should have caught this, so I don't know of a good way to test it better, but in testbed this did come up and required the fix I have implemented here.

@@ -54,8 +90,10 @@ module.exports = (self) => {
addMissingSchemaFieldsFor(doc, schema, dotPath, changes) {
for (const field of schema) {
const newDotPath = dotPath ? `${dotPath}.${field.name}` : field.name;
// Supply the default
if (doc[field.name] === undefined) {
// Supply the default if a field is undefined, and also in the
Copy link
Member Author

Choose a reason for hiding this comment

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

Not really our fault per se if the object field had a weird non-object value rather than being undefined, but it happened "in the field" with testbed, so fix it!

@@ -91,7 +129,10 @@ module.exports = (self) => {
}
} else if (field.type === 'relationship') {
for (const [ key, item ] of Object.entries(doc[field.fieldsStorage] || {})) {
const itemPath = `${newDotPath}.${field.fieldsStorage}.${key}`;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the wrong path.

@@ -101,7 +142,7 @@ module.exports = (self) => {
getPropLists() {
const schema = {};
for (const [ name, module ] of Object.entries(self.apos.doc.managers)) {
if (!module.__meta.name) {
if (!module.__meta?.name) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Fake managers have no __meta so this wasn't tolerant enough.

@haroun
Copy link
Contributor

haroun commented Sep 25, 2024

@boutell we might need to explain in the doc the new behavior,
specially for folks who change the schema and create a migration in the form

if the field value does not exist, set the value to something other than the default
(example, I want new pieces to use the default in the admin UI but I want all existing pieces to have another value using a migration)

@boutell
Copy link
Member Author

boutell commented Sep 25, 2024

Good point, we should document this in the migrations docs.

@boutell boutell merged commit e4d8b63 into main Sep 25, 2024
8 checks passed
@boutell boutell deleted the pro-6472-b branch September 25, 2024 11:41
haroun added a commit that referenced this pull request Sep 25, 2024
* main:
  fix oversights in add missing schema fields (#4736)
haroun added a commit to haroun/apostrophe that referenced this pull request Sep 26, 2024
* upstream/main:
  fix oversights in add missing schema fields (apostrophecms#4736)
  upgrate stylelint-config-apostrophe to 4.2.0 (apostrophecms#4735)
  PRO-6472: at startup, automatically supply a value for any new schema property that has a def or fallback def (apostrophecms#4721)
  fix sass warning (apostrophecms#4730)
  fix piecesFilters with dynamic choices (apostrophecms#4731)
  hotfix 4.7.1 (apostrophecms#4733)
  PRO-6591: parked fields overrides (apostrophecms#4732)
  do not let a page become a child of itself (apostrophecms#4726)
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