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

Feature/adding file attachments #108

Merged
merged 3 commits into from
Aug 14, 2017

Conversation

s-ashwinkumar
Copy link
Contributor

  1. Added File attachments so that user can create a comment with attachments or add attachments to an existing comments. Also includes delete operation on a single attachment or all attachments of a comment.
  2. Once a comment is modified, story.comment still returns the old comment as it returns from the instance variable, added an optional parameter 'reload' to re-fetch the comment.
  3. Small refactoring of client to avoid repeating methods definition for get, put, post, delete (more meta programming can be used for that class)
  4. Added specs for the features above and updated the README to show the usage of added features.

Please let me know about any corrections. I have tried to follow the same pattern as it is currently.

@coveralls
Copy link

coveralls commented Jun 12, 2017

Coverage Status

Coverage increased (+0.08%) to 96.899% when pulling 215d569 on s-ashwinkumar:feature/adding_file_attachments into dfff964 on dashofcode:master.

@s-ashwinkumar s-ashwinkumar mentioned this pull request Jun 13, 2017
@s-ashwinkumar s-ashwinkumar force-pushed the feature/adding_file_attachments branch from 215d569 to fc8776e Compare June 19, 2017 21:09
@coveralls
Copy link

coveralls commented Jun 19, 2017

Coverage Status

Coverage increased (+0.08%) to 96.899% when pulling fc8776e on s-ashwinkumar:feature/adding_file_attachments into dfff964 on dashofcode:master.

@forest
Copy link
Contributor

forest commented Jun 21, 2017

Thanks for contributing. I've been swamped but will get to this in the next week or so.

@s-ashwinkumar
Copy link
Contributor Author

@forest Thank you for letting me know !

Copy link
Contributor

@forest forest left a comment

Choose a reason for hiding this comment

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

Thanks for this addition. Lots of work and I'm sure will be appreciated by many.

Resources::FileAttachment.new({ comment: comment }.merge(data))
end

# This orphans the file, deletes source, but appears in the comments
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the commented out code?

You can put TODOs or notes in comments at the top of the class if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@forest Thank you. Sorry, for some weird reason I never got any notifications. Let me get to these this week. Will convert this to TODO

@@ -0,0 +1,16 @@
module TrackerApi
class FileUtility
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix this white space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(y)

# File.open(filename, 'wb') { |fp| fp.write(file_data) }
# end

def to_upload_hash
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this used for?

@coveralls
Copy link

coveralls commented Aug 14, 2017

Coverage Status

Coverage increased (+0.2%) to 96.972% when pulling 6c046db on s-ashwinkumar:feature/adding_file_attachments into dfff964 on dashofcode:master.

@s-ashwinkumar
Copy link
Contributor Author

@forest handled the CR changes. Thank you and sorry for the delay.

@forest
Copy link
Contributor

forest commented Aug 14, 2017 via email

@forest forest merged commit bd4639a into irphilli:master Aug 14, 2017
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