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

Implement asana_add_comment action #5

Merged
merged 15 commits into from
Sep 5, 2024
Merged

Conversation

jaceklyp
Copy link
Contributor

@jaceklyp jaceklyp commented Sep 4, 2024

@jaceklyp jaceklyp requested a review from ayoy September 4, 2024 11:11
Copy link
Contributor

@ayoy ayoy left a comment

Choose a reason for hiding this comment

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

@jaceklyp this looks good overall! Just leaving some improvement ideas for now and will look at tests now.

Comment on lines 88 to 98
if task_id.nil? && task_url.nil?
raise ArgumentError, "Both task_id and task_url cannot be nil. At least one must be provided."
end

if comment.nil? && template_name.nil?
raise ArgumentError, "Both comment and template_name cannot be nil. At least one must be provided."
end

if comment && workflow_url.nil?
raise ArgumentError, "If comment is provided, workflow_url cannot be nil"
end
Copy link
Contributor

Choose a reason for hiding this comment

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

We also don't want empty values, so please rewrite all .nil? checks in this function as .to_s.empty?
(nil.to_s equals "")

File.read(template_file)
rescue StandardError
UI.user_error!("Error: The file '#{template_name}.yml' does not exist.")
nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's needed to explicitly return nil, but I may be wrong 🤷

Comment on lines 125 to 126
processed_content = template_content.gsub(/\$\{(\w+)\}/) { ENV.fetch($1, '') }
processed_content.gsub(/\s*\n\s*/, ' ').strip
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 the following would be equivalent:

Suggested change
processed_content = template_content.gsub(/\$\{(\w+)\}/) { ENV.fetch($1, '') }
processed_content.gsub(/\s*\n\s*/, ' ').strip
template_content.gsub(/\$\{(\w+)\}/) { ENV.fetch($1, '') }
.gsub(/\s*\n\s*/, ' ')
.strip

Copy link
Contributor

@ayoy ayoy left a comment

Choose a reason for hiding this comment

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

@jaceklyp overall looks great. Please address the comments and also add 1 more test that I've outlined in one of the comments. Thanks a lot!

@@ -8,6 +9,16 @@ module Helper
class DdgAppleAutomationHelper
ASANA_API_URL = "https://app.asana.com/api/1.0"
ERROR_ASANA_ACCESS_TOKEN_NOT_SET = "ASANA_ACCESS_TOKEN is not set"

def self.load_asset_file(file)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note that this isn't strictly "loading" the file, but only constructing file path. We could have loading here, in which case we need File.read at the end, or we can keep it as is and rename to path_for_asset_file for instance (?)

end
end

it "removes newlines and leading/trailing spaces" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add one more test similar to this one? It's about removing only 1 consecutive newline, i.e.

Hello\n  \n  World

should render

Hello\nWorld

(or something similar 🤪 but we have a bunch of templates that add 2 newlines to end up with 1 newline in Asana)

Copy link
Contributor

@ayoy ayoy left a comment

Choose a reason for hiding this comment

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

LGTM, great job 💪

.gsub(/\s+/, ' ') # replace multiple whitespaces with a single space
.gsub(/>\s+</, '><') # remove spaces between HTML tags
.strip # remove leading and trailing whitespaces
.gsub(%r{<br\s*/?>}, "\n") # replace <br> tags with newlines
Copy link
Contributor

Choose a reason for hiding this comment

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

<3

@jaceklyp jaceklyp merged commit f63e985 into main Sep 5, 2024
1 check passed
@jaceklyp jaceklyp deleted the jacek/asana-add-comment branch September 5, 2024 09:05
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