Skip to content

Conversation

@fieldofsheep
Copy link

Added Expiration for Contact to Signal CLI. I haven't much of a clue of how .go works so you might need to re-run the swagger to pull the doc interface. its listed under v1 update contact and the API appears to work. User must set the expiration prior to sending a message as previous messages aren't linked to the new timeout.

Copy link
Owner

@bbernhard bbernhard left a comment

Choose a reason for hiding this comment

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

Many thanks for the PR!

I've added some feedback and improvement suggestions. All the things that I've found are just some minor issues, but I would like to address them already at this point, as breaking the API later is always a bit complicated.

log.Debug("*) su signal-api")
log.Debug("*) ", fullCmd)
log.Debug("*) ", signalCliBinary, " ", strings.Join(args, " "))

Copy link
Owner

Choose a reason for hiding this comment

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

I think you accidentally removed some code here (probably due to a merging conflict), see: https://github.com/bbernhard/signal-cli-rest-api/blob/master/src/api/api.go#L345

}

if len(req.Name) == 0 {
c.JSON(400, gin.H{"error": "Couldn't process request - please provide the recipients name number"})
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a return here

}

if len(req.Expiration) == 0 {
c.JSON(400, gin.H{"error": "Couldn't process request - please provide an expirty time in seconds"})
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a return here

type UpdateContactRequest struct {
Name string `json:"name"`
Number string `json:"number"`
Expiration string `json:"expiration"`
Copy link
Owner

Choose a reason for hiding this comment

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

I think it makes sense to change the type to int here. Maybe also make it more clear that it's a time?

so e.g something like: ExpirationTime int json:"expiration_time"``?

identities.PUT(":number/trust/:numbertotrust", api.TrustIdentity)
}

contacts := v1.Group("updatecontact")
Copy link
Owner

Choose a reason for hiding this comment

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

Personally I would prefer if we could change the endpoint to contacts, as the POST method already implies that it will be a update/change request. I think what we probably also should do here is to restrict the request to a certain signal instance number. i.e: contacts/:number. And pass that number then also with -u to the runSignalCli function.

fieldofsheep added a commit to fieldofsheep/signal-cli-rest-api that referenced this pull request Oct 29, 2021
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