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: add notification helpers #136

Merged
merged 27 commits into from
Nov 7, 2024
Merged

feat: add notification helpers #136

merged 27 commits into from
Nov 7, 2024

Conversation

gladkia
Copy link
Collaborator

@gladkia gladkia commented Oct 17, 2024

@gladkia gladkia requested review from j-smola and bczech October 17, 2024 15:38
@gladkia gladkia requested a review from a team as a code owner October 17, 2024 15:38
R/duplicates.R Outdated Show resolved Hide resolved
R/duplicates.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated
checkmate::assert_flag(html)
checkmate::assert_flag(inline)
checkmate::assert_from(host_name, min.chars = 1)
checkmate::assert_character(attached_files)
Copy link
Contributor

Choose a reason for hiding this comment

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

These two lines should be wrapped into if, to allow for using def arg with NULL, i.e.

    if (!is.null(attached_files)) {
        checkmate::assert_character(attached_files)
        checkmate::assert_file_exists(attached_files)
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done in 708cae4.

R/duplicates.R Outdated Show resolved Hide resolved
R/duplicates.R Outdated Show resolved Hide resolved
R/duplicates.R Outdated
qs::qsave(att_l, att_f)
m_to <- get_env_var("EMAIL_RECIPIENT")
stopifnot(nchar(m_to) > 0)
send_email(body = msg, to = m_to, from = from, attached_files = att_f)
Copy link
Contributor

Choose a reason for hiding this comment

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

arg from is not definied

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 1c494b0.

R/duplicates.R Outdated
if ("slack" %in% by) {
s_to <- get_env_var("EMAIL_SLACK_NOTIFICATION")
stopifnot(nchar(s_to) > 0)
send_email(body = msg, to = s_to, from = from)
Copy link
Contributor

Choose a reason for hiding this comment

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

arg from is not definied

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 1c494b0.

R/duplicates.R Outdated Show resolved Hide resolved
R/duplicates.R Outdated Show resolved Hide resolved
R/duplicates.R Outdated
m_sbj <- "[gDR] Error - unexpected duplicates found"

if ("email" %in% by) {
att_l <- list(c(dup_dt = dup_dt, metadata))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional to create one-element list instead of list(dup_dt = dup_dt, metadata = metadata)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Usually it will be a list with multiple elements because metadata usually will be a non-empty list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

R/utils.R Outdated Show resolved Hide resolved
gladkia and others added 3 commits October 18, 2024 11:48
R/duplicates.R Outdated Show resolved Hide resolved
Copy link
Contributor

@j-smola j-smola left a comment

Choose a reason for hiding this comment

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

Please, add tests.

R/duplicates.R Outdated Show resolved Hide resolved
R/duplicates.R Outdated Show resolved Hide resolved
R/duplicates.R Outdated
s_to <- get_env_var("EMAIL_SLACK_NOTIFICATION")
stopifnot(nchar(s_to) > 0)
send_email(body = msg, subject = m_sbj, to = s_to)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

a.a.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved to GC

R/utils.R Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
@gladkia gladkia merged commit b87d525 into main Nov 7, 2024
3 of 4 checks passed
@gladkia gladkia deleted the GDR-2718 branch November 7, 2024 07:27
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.

3 participants