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

feat(admin): Add custom converters for collection types #99

Merged
merged 3 commits into from
Nov 7, 2019

Conversation

benjamingeer
Copy link
Contributor

@benjamingeer benjamingeer commented Nov 6, 2019

This PR adds implementations of JsonCustomConvert to handle the collections that are objects of the properties administrativePermissionsPerProject and groupsPerProject in the class PermissionsData.

Resolves #92.
Resolves #97.
Closes #98.
Needs dasch-swiss/dsp-api#1505.

@tobiasschweizer
Copy link
Contributor

tobiasschweizer commented Nov 7, 2019

@benjamingeer This looks good, thank you! I will check this PR with @kilchenmann today.

Copy link
Contributor

@kilchenmann kilchenmann left a comment

Choose a reason for hiding this comment

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

I wanted to use the builded package of this branch in the knora-ui or in the knora-api-js-lib-test app, but I got on both branches the following errors:

ERROR in node_modules/@knora/api/src/api/endpoint.d.ts(18,9): error TS1086: An accessor cannot be declared in an ambient context.
node_modules/@knora/api/src/api/endpoint.d.ts(22,9): error TS1086: An accessor cannot be declared in an ambient context.
node_modules/@knora/api/src/knora-api-config.d.ts(18,9): error TS1086: An accessor cannot be declared in an ambient context.

The error says it's because of these lines:

    /**
     * The session token
     */
    get jsonWebToken(): string;
    /**
     * The session token
     */
    set jsonWebToken(value: string);

I cleaned everything and I'm not sure if it's an issue from this branch or from somewhere else.

@tobiasschweizer
Copy link
Contributor

I wanted to use the builded package of this branch in the knora-ui or in the knora-api-js-lib-test app, but I got on both branches the following errors:

I have spotted this behavior as well in dasch-swiss/knora-api-js-lib-test#21 when the e2e test ran on travis.

I could reproduce this locally with yarn when building @knora/api.

Please try the following:

  • run npm i in your @knora/api project root
  • run yalc remove --all
  • run npm run yalc-publish

In @knora/api we use npm and have an package-lock.json, but there is not yarn.lock.

Then install the local package in your app with yarn

@kilchenmann
Copy link
Contributor

I could reproduce this locally with yarn when building @knora/api.

Thanks @tobiasschweizer. It's seems to be an issue with the dependencies. npm install fixed it because of missing PhantomJS and core-js. I thought last time it wasn't a problem with yarn. But you're right, we should use npm in this library. Now I can review it..

@kilchenmann kilchenmann self-requested a review November 7, 2019 11:19
Copy link
Contributor

@kilchenmann kilchenmann left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks

@benjamingeer
Copy link
Contributor Author

Thanks @tobiasschweizer and @kilchenmann for your help with this!

@benjamingeer benjamingeer merged commit 93b5fc6 into master Nov 7, 2019
@benjamingeer benjamingeer deleted the fix/92-permissions-data branch November 7, 2019 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use json2typescript when submitting payloads in admin api Users endpoint: missing values
3 participants