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(sync-translated-content): sync case changes + move adjacent files #7886

Merged
merged 13 commits into from
Feb 17, 2023

Conversation

yin1999
Copy link
Member

@yin1999 yin1999 commented Jan 1, 2023

Summary

Fixes: #7778

Problem

  1. sync-translated-content didn't sync slug case changes
  2. sync-translated-content didn't move all adjacent files, if a content page moved.

Solution

  1. Read slug from mdn/content and skip the content move part if only changed slug case.
  2. add case change as a part of redirect file modification (we can also use yarn tool to move content if only changed slug case)
  3. Find and move all the adjacent files when the document is moved.

Screenshots

After

image
image
image


How did you test this change?

Run yarn tool sync-translated-content zh-cn and yarn move (case changed only) to verify.

tool/sync-translated-content.ts Outdated Show resolved Hide resolved
content/redirect.ts Outdated Show resolved Hide resolved
@yin1999 yin1999 requested a review from caugner February 7, 2023 10:26
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Running yarn tool sync-translated-content zh-cn with content @ 04775ddbbdb965b02f20a71461268084d1efd7c1 and an outdated translated-content @ f339ce907032d79788eab03303d33cba328a9ea7 (2022-12-22), I got this:

% yarn tool sync-translated-content zh-cn
yarn run v1.22.19
$ ts-node tool/cli.ts sync-translated-content zh-cn

error: ENOENT: no such file or directory, open '/Users/claas/github/mdn/translated-content/files/zh-cn/orphaned/games/examples/index.md'

error Command failed with exit code 1.

Context:

fs.writeFileSync(filePath, combined);

Running the script with yari @ main works fine:

% yarn tool sync-translated-content zh-cn
yarn run v1.22.19
$ ts-node tool/cli.ts sync-translated-content zh-cn
Syncing zh-cn:
Total of 5114 documents
Moved 13 documents
Conflicting 3 documents.
Orphaned 3 documents.
Fixed 10 redirected documents.
✨  Done in 12.33s.

@caugner
Copy link
Contributor

caugner commented Feb 7, 2023

@yin1999 db91322 should resolve the error I was getting. Does that change look good to you?

@yin1999
Copy link
Member Author

yin1999 commented Feb 8, 2023

@yin1999 db91322 should resolve the error I was getting. Does that change look good to you?

Thanks @caugner. It looks good to me :)

@github-actions github-actions bot added the merge conflicts 🚧 Please rebase onto or merge the latest main. label Feb 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label Feb 8, 2023
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe we can replace filePathChanged by a proper status flag and fix the semantics of `

tool/sync-translated-content.ts Outdated Show resolved Hide resolved
@yin1999 yin1999 requested a review from caugner February 8, 2023 15:16
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, just one more opportunity. :)

tool/cli.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

@yin1999 Just to double-check. This PR fixes two issues, right?

  1. We now sync case changes (previously we didn't).
  2. We now also move all adjacent files, if a content page moved (previously we didn't).

Is that accurate, or did I miss anything?

@yin1999
Copy link
Member Author

yin1999 commented Feb 9, 2023

@yin1999 Just to double-check. This PR fixes two issues, right?

  1. We now sync case changes (previously we didn't).
  2. We now also move all adjacent files, if a content page moved (previously we didn't).

Yes

@caugner caugner changed the title fix(tool): sync the case changes of slug for content sync fix(sync-translated-content): sync case changes + move adjacent files Feb 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2023

Dev build for 2bcc01a was deployed to: https://pr7886.content.dev.mdn.mozit.cloud/

@caugner caugner merged commit 73e8560 into mdn:main Feb 17, 2023
@caugner caugner added wx writer experience 🧑‍🤝‍🧑 community contributions by our wonderful community labels Feb 17, 2023
@yin1999 yin1999 deleted the sync-case branch February 17, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧑‍🤝‍🧑 community contributions by our wonderful community wx writer experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sync-translated-content tool won't sync the case change of slug
2 participants