Repository navigation
Conversation
|
The coverage job on e2c5b0bf passed 313 tests with 16 pending, including the new webpage regression. The six Node 20–25 lint/test jobs also passed. The failure is in This leaves coverage-comment publication blocked by the workflow token permissions; I found no PR-code change needed for this failure. |
|
The failing Test and report job is not a test failure: the coverage-comment action (sidx1024/report-nyc-coverage-github-action) dies with |
|
hi Jeremy, i dont understand this PR nor the underlying issue. i tested myself and couldnt reproduce your problem. please let me know. happy to fix whats actually broken but I cant merge this PR without more details |
|
@JoernBerkefeld I overstated the scope in the description. The condition I reproduced is the metadata/file shape, not the asset's creation date:
This runs locally from the repository root after // Save as repro.mjs, then run node repro.mjs
import './lib/index.js';
import Asset from './lib/metadataTypes/Asset.js';
import { mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises';
import { tmpdir } from 'node:os';
import { join } from 'node:path';
const dir = await mkdtemp(join(tmpdir(), 'sfmc-webpage-'));
try {
await mkdir(join(dir, 'asset/asset/repro'), { recursive: true });
await writeFile(join(dir, 'asset/asset/repro/content.asset-asset-meta.html'),
'<p>No content-block references</p>');
const item = { customerKey: 'repro', assetType: { name: 'webpage' } };
console.log(Object.keys(await Asset._getUpsertOrderAndSkipMissing({ repro: item }, dir)));
} finally {
await rm(dir, { recursive: true, force: true });
}On current base |
|
I changed your base branch to develop. That, among other things changes where webpage type is stored (prep for v10). What did you mean by "I have not verified this against a live SFMC tenant or established that all assets created after 2022 have that shape." Also, could you provide a sample mcdev command that you ran which would cause this issue? Retrieve and deploy worked in my test. Did you see it with build*? |
|
To be clear, your code sample above is not what I'm looking for. I need a real end-2-end |
e2c5b0b to
ee4e0cd
Compare
|
@JoernBerkefeld thanks for retargeting. I've rebased onto What "not verified against a live tenant" meant: I don't have an SFMC tenant. I reproduced it at the unit level from the asset shape @rlilov described in #2632, not from my own end-to-end run. I can't give you a live log myself. Command: the steps are the ones @rlilov reported on 9.0.3 in #2632: Why retrieve + deploy can pass in a test: the throw needs two things together:
On deploy, the pre-deploy reference check calls I checked this on today's @rlilov, if you can run your 2022+ webpage through |
|
@shoemoney , Tested end-to-end against a live SFMC tenant (child BU, sandbox) on Windows 11 / Node 24.18.0. The fix works. Setup
Run 1: Run 2: PR #2648 A retrieve after the deploy returns the v2 HTML in |
|
Other things I saw (not blockers for this PR)
|
|
@rlilov thanks for the live-tenant deployment comparison and the retrieve-after-deploy confirmation. That provides the tenant evidence I could not run myself; your fixture conversion also makes the tested metadata shape explicit. I addressed the test feedback in 9d33385: the temporary directory is now relative, avoiding the drive colon that the production path filter encodes on Windows, and the fixture uses the current Validation here: 28 asset tests passed with Node 22.22.3, and ESLint passed for the changed test file. I have not run the revised test on Windows; that platform confirmation is still outstanding. The saved-session scope rejection is separate from this patch. A separate issue with the sanitized error and SDK version would help keep that investigation focused. |
|
@rlilov - so you are saying the cloudpages created after 2022 have a different layout? ive checked this on multiple instances of SFMC and was not able to reproduce it which puzzles me quite a bit. @rlilov please understand that this project is used by hundreds of people and some SFMC instances produce other output then others for some reason. therefore i need to be careful to not break things for other users with this patch. thank you both for your contribution this far. lets push a bit more to close this. |
|
@JoernBerkefeld thanks, here is the real server response. Command (mcdev 9.0.3, child BU): mcdev retrieve / -m asset: --api log { %%=v(@Greeting)=%% World \n\n\n","availableViews": [], "modelVersion": 2 } The query response for the same asset has the same shape: content at the top level, availableViews: [], and no views, meta or data. About "2022+": I took the label from the // +++ new webpages / 2022+ +++ comment in Asset._mergeCode. The creation date is not the trigger. In the same tenant: Limitation: I have access to one SFMC tenant only, so I cannot say how common this shape is. The fix changes only the content.html branch (the 2022+ block). That block runs only when the asset has a top-level content. The views.html.content branch is unchanged, so assets with views behave as before. |
|
@JoernBerkefeld I replaced the constructed fixture with @rlilov’s recorded response and added regressions for extraction, merge, dependency scanning, and the existing views layout. The asset tests and lint pass; the two full-suite init-tooling failures also occur on the original PR head. I removed the creation-year labels because the response shape varies by BU. I haven’t tested this update on Windows or a live tenant. |
|
Thank you. |
…nt for 2022+ webpages _mergeCode read metadata.views.html.content when building the fileList entry for new-style (2022+) webpage assets, but those assets have no views object at all - content lives at metadata.content, exactly as the sibling 'template' case already does a few lines up. This throws a TypeError (Cannot read properties of undefined, reading 'html') whenever templateName is set, which happens during a normal deploy via Asset.replaceCbReference(). The asset gets silently skipped with a misleading 'content block ... was not found' error instead.
eeae2ca to
0a3d79e
Compare
|
Rebased onto current |
PR details
What changes did you make? (Give an overview)
Addresses #2632. A webpage with top-level
contentand noviewscan be skipped during deployment:_mergeCodeloads its HTML intometadata.content, then readsmetadata.views.html.contentwhen building the file list. This throws and is reported as a missing content block.Use
metadata.contentfor that entry. Theviews.html.contentpath keeps its existing behavior.Further details (optional)
The regression uses rlilov's sanitized server response. It checks extraction, merge and the deployment dependency scan. A second test covers the existing
views.html.contentfixture.This is a metadata-shape issue. The response shape differs between BUs in the reporter's tenant; creation year does not identify affected assets.
Validation: 29 asset tests passed on Node 22; both webpage regressions passed on Node 24; changed-file ESLint passed. Restoring the original file-list read makes the recorded-response test fail with the expected TypeError. The full suite had 554 passing, 21 pending and two init-tooling failures; both failures also occur with the original PR-head files. I have not run the updated tests on Windows or against a live SFMC tenant. The reporter's earlier tenant comparison used converted local files; the later comment supplies the actual server response used here.
AI assistance was used to update the tests and draft this description.
Checklist