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.

Sign up for GitHub

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

[184790111]: Redirect Crunch Automation on root project folder #621

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[184790111]: Redirect Crunch Automation on root project folder #621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions R/automation.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,18 +206,21 @@ setMethod("sendCrunchAutomationScript", "ProjectFolder", function(x,
# which gives us the URL to hit;
# but the account ('top-level folder', what you get from: `projects()`)
# is also of class ProjectFolder, but doesn't include this info;
# running CA scripts on the account is not supported currently
# previously, we function raised an error in this case;
# now, following what frontend did, we execute the script on the account
if (!is.crunchURL(x@views$execute)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should do some heuristics to only redirect when at the root and keep the error if we're not and there's no execute view (I don't know if it currently works this way, but I can imagine the API not letting you run scripts on certain folders because of permissions by not including the execute view, and we wouldn't want to silently redirect to the account in this case).

Is there an obvious way to tell? If not, we can ask Jose what the frontend does here: https://github.com/Crunch-io/whaam/pull/8006/files#diff-ed5a738f637f797570efe80b7388b58ef741de4cb66bf7615caf498b3b35978bR27

halt(
"This folder does not support Crunch Automation scripts at this time."
x <- ShojiObject(
crGET(
shojiURL(getAPIRoot(), "views", "account")
)
)
}

dots <- list(...)
if (length(dots) > 0) {
# could have been a warning, but went with error in case a user
# would try running a destructive operation with dry_run = TRUE
stop("extra arguments (...) are not supported when x is a ProjectFolder")
halt("extra arguments (...) are not supported when x is a ProjectFolder")
}

crPOST(
Expand Down
6 changes: 6 additions & 0 deletions tests/testthat/app.crunch.io/api/account.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is how I was imagining getting around the error in GET before the expect_POST. It seems like it works for me, do you still get errors? (If so, did you run make compress-fixtures?)

"element": "shoji:entity",
"views": {
"execute": "https://app.crunch.io/api/account/execute/"
}
}
19 changes: 12 additions & 7 deletions tests/testthat/test-automation.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,14 +282,19 @@ with_mock_crunch({
)
})

test_that("folder-level operation fails on root", {
test_that("folder-level operation works on root/account", {

root_project_folder <- projects()
script <- "CREATE FOLDER 'My not-to-be folder';"

expect_error(
runCrunchAutomation(root_project_folder, script),
"not support Crunch Automation scripts"
project_folder <- projects()
script <- "CREATE FOLDER 'My to-be folder';"
expected_url <- "https://app.crunch.io/api/account/execute/"
expected_body <- paste0(
'{"element":"shoji:view",',
paste0('"value":', '"', script, '"'), '}'
)
expect_POST(
runCrunchAutomation(project_folder, script),
expected_url, expected_body,
fixed = TRUE
)
})

Expand Down