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(api): add emergency contacts api #1082

Open
wants to merge 4 commits into
base: feat/add-emergency-contacts-database-functions
Choose a base branch
from

Conversation

almostinf
Copy link
Member

@almostinf almostinf commented Sep 10, 2024

Add emergency contacts api

Added API for working with emergency contacts - these are the contacts to which notifications will go to users in case of emergency types of problems in Moira

@almostinf almostinf requested a review from a team as a code owner September 10, 2024 11:03
for _, contact := range contacts {
if contact != nil {
userSettings.Contacts = append(userSettings.Contacts, *contact)
}
}

emergencyContacts, err := database.GetEmergencyContactsByIDs(contactIDs)
Copy link
Member

Choose a reason for hiding this comment

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

Это мы по ID обычных контактов получаем список эмёрдженси-контактов? Нейминг немного неочевидный 😓

Copy link
Member Author

@almostinf almostinf Sep 23, 2024

Choose a reason for hiding this comment

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

Да, так как 1-1 связь. Мне показалось, что не хорошо будет называть, как emergencyContactIDs, так как будет складываться впечатления, что у emergencyContact есть свои айдишники, а это не так

"fmt"
"net/http"

"github.com/moira-alert/moira"
)

var (
errEmptyContactType = errors.New("contact type can not be empty")
errUserLoginAndTeamIDFilledIn = errors.New("contact cannot have both the user field and the team_id field filled in")
Copy link
Member

@Tetrergeru Tetrergeru Sep 20, 2024

Choose a reason for hiding this comment

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

Suggested change
errUserLoginAndTeamIDFilledIn = errors.New("contact cannot have both the user field and the team_id field filled in")
errUserLoginAndTeamEmpty = errors.New("contact cannot have both the user field and the team_id field filled in")

Copy link
Member Author

Choose a reason for hiding this comment

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

А почему такой нейминг? В ошибке ведь наоборот написано, что оба поля заполнены, а не пустые


// nolint: gofmt,goimports
//
// @summary Gets all Moira emergency contacts
Copy link
Member

Choose a reason for hiding this comment

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

Это прям вообще все-все или для текущего юзера? Если прям совсем все -- может за админкой закроем?

Copy link
Member

Choose a reason for hiding this comment

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

Или так и есть, а я не заметил

Copy link
Member Author

Choose a reason for hiding this comment

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

Закрыто админкой и так

// @failure 422 {object} api.ErrorRenderExample "Render error"
// @failure 500 {object} api.ErrorInternalServerExample "Internal server error"
// @router /emergency-contact/{contactID} [get]
func getEmergencyContactByID(writer http.ResponseWriter, request *http.Request) {
Copy link
Member

Choose a reason for hiding this comment

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

Тут мы, как будто, тоже пользователя не проверяем...

Copy link
Member Author

Choose a reason for hiding this comment

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

Так и не нужно проверять, используется contactFilter, так как 1-1 связь между контактами

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.

4 participants