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.

Sign up for GitHub

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

Jump to bottom

fix: CPE loading changes from backend and not from workspace #2429

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

fix: CPE loading changes from backend and not from workspace #2429

wants to merge 22 commits into from

Conversation

mmilko01
Copy link
Contributor

@mmilko01 mmilko01 commented Oct 1, 2024 •
edited
Loading

#2445

  • Adds the merged manifest in applicationDependencies of the sap-ushell-config in the html
  • Sets asyncHints.requests to an empty string for SAPUI5 versions lower than 1.72
  • Introduces a workspacePath URL parameter to the LREP request to allow for correctly setting the workspace path in case of Adaptation Project

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Oct 1, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 5589a9d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
@sap-ux/adp-tooling Patch
@sap-ux/axios-extension Patch
@sap-ux-private/preview-middleware-client Patch
@sap-ux/preview-middleware Patch
@sap-ux/create Patch
@sap-ux/abap-deploy-config-inquirer Patch
@sap-ux/app-config-writer Patch
@sap-ux/backend-proxy-middleware Patch
@sap-ux/deploy-tooling Patch
@sap-ux/environment-check Patch
@sap-ux/odata-service-inquirer Patch
@sap-ux/system-access Patch
@sap-ux/odata-cli Patch
@sap-ux/generator-simple-fe Patch
@sap-ux/abap-deploy-config-writer Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

tobiasqueck
tobiasqueck requested changes Oct 7, 2024
View reviewed changes
Copy link
Contributor

@tobiasqueck tobiasqueck left a comment

Choose a reason for hiding this comment

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

Please see my inline comments

packages/axios-extension/src/abap/lrep-service.ts Outdated Show resolved Hide resolved
packages/preview-middleware-client/src/flp/bootstrap.js Show resolved Hide resolved
packages/preview-middleware/src/base/config.ts Show resolved Hide resolved
@mmilko01 mmilko01 marked this pull request as ready for review October 8, 2024 11:45
@mmilko01 mmilko01 requested review from a team as code owners October 8, 2024 11:45
nikmace
nikmace requested changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@nikmace nikmace left a comment

Choose a reason for hiding this comment

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

Test snapshots do not reflect the changes I think, I only see {\"manifest\":true}}, but in the case of ADP there should be the descriptor, or at least its mock.

Please also add a short description of the changes in this PR.

packages/axios-extension/src/abap/lrep-service.ts Show resolved Hide resolved
packages/preview-middleware-client/src/flp/bootstrap.js Outdated Show resolved Hide resolved
@nikmace nikmace self-requested a review October 10, 2024 06:24
nikmace
nikmace previously approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@nikmace nikmace left a comment

Choose a reason for hiding this comment

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

Code looks good
Comments addressed, test for adding descriptor added
Excellent coverage
Did not test manually

tobiasqueck
tobiasqueck previously approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@tobiasqueck tobiasqueck left a comment

Choose a reason for hiding this comment

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

review feedback applied as requested.

@mmilko01 mmilko01 dismissed stale reviews from tobiasqueck and nikmace via d78eef9 October 16, 2024 07:27
heimwege
heimwege approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@heimwege heimwege left a comment

Choose a reason for hiding this comment

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

Code changes look reasonable
All remarks resolved
awesome test coverage
Did not test manually

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 17, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heimwege heimwege heimwege approved these changes

@tobiasqueck tobiasqueck tobiasqueck left review comments

@nikmace nikmace Awaiting requested review from nikmace

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mmilko01 @nikmace @tobiasqueck @heimwege