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

Update Users Service to use Typescript #7

Merged
merged 19 commits into from
Apr 4, 2024
Merged

Conversation

Musilah
Copy link
Contributor

@Musilah Musilah commented Mar 25, 2024

What does this do?

Update Users Service to use Typescript

Which issue(s) does this PR fix/relate to?

Resolves #12

List any changes that modify/break current functionality

  • modify sdk.ts, errors.ts, examples.ts and users.ts

Have you included tests for your changes?

No

Did you document any new/modified functionality?

No

Notes

@Musilah Musilah marked this pull request as ready for review March 26, 2024 13:39
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
src/users.ts Outdated
'Content-Type': this.contentType,
Authorization: `Bearer ${token}`
},
body: JSON.stringify(user)
Copy link
Contributor

Choose a reason for hiding this comment

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

Magistrala only requires a string called identity, not the whole user. You can do sth like:

Suggested change
body: JSON.stringify(user)
body: JSON.stringify({ identity: user.credentials?.identity })

Copy link
Contributor

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

Remove .vscode from the PR. You can add it to your global .gitignore so you keep it for yourself.

tsconfig.json Show resolved Hide resolved
tsup.config.ts Show resolved Hide resolved
src/users.ts Outdated
const errorRes = await response.json()
throw this.userError.HandleError(errorRes.error, response.status)
}
return 'User Disabled'
Copy link
Contributor

Choose a reason for hiding this comment

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

It should return a user back

src/users.ts Outdated
const errorRes = await response.json()
throw this.userError.HandleError(errorRes.error, response.status)
}
return 'User Enabled'
Copy link
Contributor

Choose a reason for hiding this comment

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

return a user back

@@ -0,0 +1,210 @@
import SDK from '../src/sdk'
Copy link
Contributor

Choose a reason for hiding this comment

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

lets name the file users.ts, since we are already in the examples directory


const defaultUrl = 'http://localhost'

const mySdk = new SDK({
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this file since the code will be de-structured to the individual files

src/users.ts Outdated
}

interface Groups {
groups: Groups[]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct since this should be an array of values of type Group not Groups. Meaning the Group type should be imported from the groups.ts file. Same case for things and channels.

Also check the list user things/channels/groups functions, they don't seem to be functioning. I am getting this error:

SyntaxError: Unexpected non-whitespace character after JSON at position 4
    at JSON.parse (<anonymous>)
    at parseJSONFromBytes (node:internal/deps/undici/undici:5329:19)
    at successSteps (node:internal/deps/undici/undici:5300:27)
    at fullyReadBody (node:internal/deps/undici/undici:1447:9)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async specConsumeBody (node:internal/deps/undici/undici:5309:7)

src/users.ts Outdated
const errorRes = await response.json()
throw this.userError.HandleError(errorRes.error, response.status)
}
const userData = await response.json()
Copy link
Contributor

Choose a reason for hiding this comment

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

Enforce type for these responses. for example this should be

Suggested change
const userData = await response.json()
const userData : User = await response.json()

do sth similar for the rest

@@ -0,0 +1,724 @@
import Errors from './errors'
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you have this file in this pr if it's just going to be having commented code. If it's being fixed in another pr remove it from this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some users.ts functions need the things.ts interfaces and url so I added it. But I will remove the commented out code

@@ -0,0 +1,59 @@
import Users from './users'
import Things from './things'
Copy link
Contributor

Choose a reason for hiding this comment

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

where are you using things in this pr?
Also remove the commented out code. It will be added in subsequent pr's as needed.

src/users.ts Outdated
page: PageRes
}

interface Group {
Copy link
Contributor

Choose a reason for hiding this comment

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

These interfaces should be imported from their respective files not created here and then being created in the groups file again. Same case for things, channels, and groups.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we can have a common defs.ts file where all the interfaces are defined and other files can import these interfaces from therer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since this is just a users.ts PR defining the interfaces here should be okay since the other files are still in .js format. Let me add a defs.ts file instead for more structure.

src/users.ts Outdated
* }
*/

const rpr = { password, confPass, token }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const rpr = { password, confPass, token }

src/users.ts Outdated
headers: {
'Content-Type': this.contentType
},
body: JSON.stringify(rpr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
body: JSON.stringify(rpr)
body: JSON.stringify( { password : password, confirm_password : confPass, token : token })

src/users.ts Outdated
headers: {
'Content-Type': this.contentType
},
body: email
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a json object

Suggested change
body: email
body: JSON.stringify( { email: email } )

src/users.ts Outdated
*
*/

const secret = { old_secret: oldSecret, new_secret: newSecret }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const secret = { old_secret: oldSecret, new_secret: newSecret }

src/users.ts Outdated
'Content-Type': this.contentType,
Authorization: `Bearer ${token}`
},
body: JSON.stringify(secret)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
body: JSON.stringify(secret)
body: JSON.stringify( { old_secret: oldSecret, new_secret: newSecret } )

src/users.ts Outdated
domain_id?: string
}

interface QueryParams {
Copy link
Contributor

Choose a reason for hiding this comment

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

These are not the only query params available. Check the pageMetadata in go-sdk and add those as the query params

src/users.ts Outdated
limit: number
}

interface UsersInterface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
interface UsersInterface {
interface UsersPage {

src/things.ts Outdated
}
owner: string
metadata: {
domain: string

Choose a reason for hiding this comment

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

Suggested change
domain: string
metadata?: Record<string, any>;

src/users.ts Outdated
Comment on lines 15 to 16
updatedAt?: string
updatedBy?: string

Choose a reason for hiding this comment

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

Suggested change
updatedAt?: string
updatedBy?: string
metadata?: Record<string, any>;

@@ -24,14 +24,22 @@ export default class Users {
* @returns {Object} - Users object.
*/
private readonly usersUrl: URL
private readonly thingsUrl: URL
private readonly thingsUrl?: URL
private readonly hostUrl: URL
Copy link
Contributor

Choose a reason for hiding this comment

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

Host url is optional too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I make hostUrl optional then the HeadersInit flags it as type undefined

this.thingsUrl = new URL(thingsUrl)
if (thingsUrl !== undefined) {
this.thingsUrl = new URL(thingsUrl)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

have an else statement for thingsurl too

src/users.ts Outdated
const errorRes = await response.json()
throw this.userError.HandleError(errorRes.error, response.status)
}
const resetRequestResponse: Response = { status: response.status, message: 'Email with reset link is sent' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const resetRequestResponse: Response = { status: response.status, message: 'Email with reset link is sent' }
const resetRequestResponse: Response = { status: response.status, message: 'Email with reset link sent successfully' }

src/users.ts Outdated
const errorRes = await response.json()
throw this.userError.HandleError(errorRes.error, response.status)
}
const resetResponse: Response = { status: response.status, message: 'Password reset succesfully' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const resetResponse: Response = { status: response.status, message: 'Password reset succesfully' }
const resetResponse: Response = { status: response.status, message: 'Password reset successfully' }

src/users.ts Outdated
/**
* @method ResetPasswordRequest - Sends a request
* @param {String} email - User email.
* @returns {Int} - Status.
Copy link
Contributor

Choose a reason for hiding this comment

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

fix the comments they are wrong

src/users.ts Outdated
* @param {String} password - User Password.
* @param {String} confPass - User to confirm the Password.
* @param {String} token - Access token.
* @returns {Int} - Status Created.
Copy link
Contributor

Choose a reason for hiding this comment

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

fix the comments

src/defs.ts Outdated
refreshToken: string
}

export interface Status {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this being used anywhere?

Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Copy link
Contributor

@ianmuchyri ianmuchyri left a comment

Choose a reason for hiding this comment

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

LGTM

@arvindh123
Copy link

LGTM

@dborovcanin dborovcanin changed the title NOISSUE - Update Users Service to use Typescript Update Users Service to use Typescript Apr 4, 2024
@dborovcanin dborovcanin merged commit 0f24bc5 into absmach:main Apr 4, 2024
1 check passed
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.

Update JS SDK for Users
4 participants