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

update(FF): Updates FF Worklist controllers and tests #63

Merged
merged 12 commits into from
Sep 29, 2021
Merged

update(FF): Updates FF Worklist controllers and tests #63

merged 12 commits into from
Sep 29, 2021

Conversation

IainSAP
Copy link
Contributor

@IainSAP IainSAP commented Sep 23, 2021 •
edited
Loading

Adds odata version agnostic controllers for FF worklist + test

@IainSAP IainSAP self-assigned this Sep 23, 2021
@IainSAP IainSAP marked this pull request as ready for review September 24, 2021 08:14
@IainSAP IainSAP changed the title WIP(FF): Adds v4 controllers required for Worklist v4 nav + test WIP(FF): Worklist v4 + test Sep 24, 2021
devinea
devinea reviewed Sep 24, 2021
View reviewed changes
Copy link
Member

@devinea devinea left a comment

Choose a reason for hiding this comment

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

@IainSAP - Can you update the description #63 (comment)? Not v4 specific any more.

@IainSAP IainSAP changed the title WIP(FF): Worklist v4 + test WIP(FF): Worklist controllers and tests Sep 24, 2021
@IainSAP IainSAP changed the title WIP(FF): Worklist controllers and tests update(FF): Updates FF Worklist controllers and tests Sep 24, 2021
@IainSAP IainSAP closed this Sep 24, 2021
@IainSAP IainSAP reopened this Sep 24, 2021
tobiasqueck
tobiasqueck requested changes Sep 27, 2021
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 the inline comments. It looks like a handful of copy & paste issues happened in which templating strings have been replaced with hardcoded values.

packages/fiori-freestyle/templates/worklist/add/webapp/controller/Worklist.controller.js Outdated Show resolved Hide resolved
packages/fiori-freestyle/templates/worklist/add/webapp/controller/Worklist.controller.js Outdated Show resolved Hide resolved
packages/fiori-freestyle/templates/worklist/add/webapp/controller/Object.controller.js Outdated Show resolved Hide resolved
packages/fiori-freestyle/templates/worklist/add/webapp/controller/Object.controller.js Outdated Show resolved Hide resolved
@tobiasqueck
Copy link
Contributor

Not required but if you add client: 100 to the test config at https://github.com/SAP/open-ux-tools/blob/feature/v4_controllers_worklist/packages/fiori-freestyle/test/worklist.test.ts#L55 then the generated app works when you keep it using the DEBUG mode.

tobiasqueck
tobiasqueck previously approved these changes Sep 28, 2021
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.

  • reviewed the fix of the clone method
  • validated that the generated worklist templates work for both v2/v4
  • didn't validate that the removal of the escaping in the ejs templates works as expected but discussed with @IainSAP and here explained that it is needed
  • ran the tests

All in all, it looks very good and is a great step forward simplifying our templates.

@tobiasqueck tobiasqueck mentioned this pull request Sep 28, 2021
Closed
devinea
devinea previously approved these changes Sep 28, 2021
View reviewed changes
Copy link
Member

@devinea devinea left a comment

Choose a reason for hiding this comment

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

Changes look good.
Backward compatibility with tool-suite checked elsewhere by @tobiasqueck

packages/fiori-freestyle/package.json Outdated Show resolved Hide resolved
packages/fiori-freestyle/src/index.ts Outdated Show resolved Hide resolved
@ullasholla @devinea
chore(ff): add dep to lodash instead of lodash.deepClone
a5506c5 
Co-authored-by: Austin Devine <devinea@users.noreply.github.com>
@ullasholla ullasholla dismissed stale reviews from devinea and tobiasqueck via a5506c5 September 28, 2021 20:49
@ullasholla @devinea
chore(ff): use lodash/cloneDeep instead of lodash.cloneDeep
bdefec1 
Co-authored-by: Austin Devine <devinea@users.noreply.github.com>
devinea
devinea previously approved these changes Sep 28, 2021
View reviewed changes
Copy link
Member

@devinea devinea left a comment

Choose a reason for hiding this comment

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

Thanks @ullasholla

@devinea
Copy link
Member

devinea commented Sep 28, 2021

Some git conflicts

tobiasqueck
tobiasqueck approved these changes Sep 29, 2021
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.

Approved (again) since the additional change only took care of merges to package.json and pnpm-lock.yaml

@tobiasqueck tobiasqueck merged commit e90dbdd into main Sep 29, 2021
@tobiasqueck tobiasqueck deleted the feature/v4_controllers_worklist branch September 29, 2021 15:37
devinea added a commit that referenced this pull request Oct 5, 2021
@devinea
Merge branch 'main' into integration_test
6edf532 
* main:
  update(FF): Updates FF Worklist controllers and tests (#63)
  fix: app-id creation at scaffolding time in Component.js (fixes #70) (#77)
  Add Node V16 build and compatibility (#64)
  chore: set version of all modules to v0.9.0 (#71)
  chore: add github codeowners file (#58)

# Conflicts:
#	.github/workflows/pipeline.yml
#	packages/fiori-freestyle/package.json
#	pnpm-lock.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlunets nlunets nlunets left review comments

@unseen1980 unseen1980 unseen1980 approved these changes

@tobiasqueck tobiasqueck tobiasqueck approved these changes

@ullasholla ullasholla Awaiting requested review from ullasholla

@devinea devinea Awaiting requested review from devinea

Assignees

@IainSAP IainSAP

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@IainSAP @tobiasqueck @devinea @ullasholla @unseen1980 @nlunets