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

[REVIEW] Reviewing implementation #7

Closed
wants to merge 1 commit into from
Closed

[REVIEW] Reviewing implementation #7

wants to merge 1 commit into from

Conversation

danjoa
Copy link
Contributor

@danjoa danjoa commented Jun 29, 2023

DON'T MERGE YET

→ let's use this for a joint review



to decide/ do before release of 0.1.0

optional

  • persistent outbox + oauth2 plan, should be solvable when accessing API without client lib
  • [ ] provider audit logs? -> currently in discussion, follow-ups as needed

@danjoa danjoa changed the title REVIEW: Reviewing implementation [REVIEW:] Reviewing implementation Jun 29, 2023
srv/service.js Show resolved Hide resolved
srv/service.js Show resolved Hide resolved
srv/service.js Show resolved Hide resolved
lib/utils.js Show resolved Hide resolved
lib/utils.js Show resolved Hide resolved
lib/utils.js Show resolved Hide resolved
cds-plugin.js Show resolved Hide resolved
cds-plugin.js Show resolved Hide resolved
cds-plugin.js Show resolved Hide resolved
@@ -5,67 +5,62 @@ const { augmentContext, calcMods4Before, calcMods4After, emitMods } = require('.
const { hasPersonalData } = require('./lib/utils')

// TODO: why does cds.requires.audit-log: false in sample package.json not work ootb?!
// REVIST: It does, doesn't it?
Copy link
Contributor

Choose a reason for hiding this comment

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

cds.env.requires['audit-log'] is false, but a cds.connect.to('audit-log') still works, so audit-logging is not "deactivated" (calculated in any case is an additional issue)...

Copy link
Contributor

@sjvans sjvans Jul 4, 2023

Choose a reason for hiding this comment

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

connect does cds.requires || cds.requires.kinds

cds-plugin.js Show resolved Hide resolved
srv/service.js Show resolved Hide resolved
index.cds Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
@sjvans sjvans changed the title [REVIEW:] Reviewing implementation [REVIEW] Reviewing implementation Jul 5, 2023
@sjvans
Copy link
Contributor

sjvans commented Jul 19, 2023

review decisions applied in #8

@sjvans sjvans closed this Jul 20, 2023
@sjvans sjvans deleted the review branch July 28, 2023 09:19
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