-
Notifications
You must be signed in to change notification settings - Fork 43
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
Update in attachment proccess #321
base: bug/318-file-uploads
Are you sure you want to change the base?
Update in attachment proccess #321
Conversation
/// <param name="filePath">The path of the file to attach.</param> | ||
/// <param name="ct">(Optional) A cancellation token for async processing.</param> | ||
/// <returns>The newly created <see cref="IBoardBackground"/>.</returns> | ||
Task<IBoardBackground> Add(string filePath, CancellationToken ct = default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of changing this, can you just add the new method, please? This will allow us to avoid a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, its seems to be a great idea
Manatee.Trello/Rest/RestFile.cs
Outdated
/// </summary> | ||
public byte[] ContentBytes { get; set; } | ||
public string FilePath { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here: just add the property.
Manatee.Trello/Rest/WebApiClient.cs
Outdated
|
||
return formData; | ||
var fileStream = File.Open(request.File, FileMode.Open); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you will need to check whether raw bytes have been uploaded or just a filename; then act appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll move forward with this, but I don't want to lose existing functionality. Just a few suggestions on how to add this without creating breaking changes.
Manatee.Trello/Rest/WebApiClient.cs
Outdated
var fileStream = File.Open(request.File, FileMode.Open); | ||
var fileInfo = new FileInfo(request.File); | ||
formData.Add(new StreamContent(fileStream), "\"file\"", string.Format("\"{0}\"", "as" + fileInfo.Extension)); | ||
formData.Add(new StringContent("mimeType"), "image/png"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have some simple functionality that attempts to infer the mime type from the file extension. Not looking for an extensive list; more like a "top 10."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments. Also tabs, not spaces, please.
@@ -21,8 +21,9 @@ public async Task Issue205_CopyCardWithAttachment() | |||
var list = board.Lists.Last(); | |||
var cards = list.Cards; | |||
var sourceCard = await TestEnvironment.Current.BuildCard(); | |||
var jpeg = Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "Files/smallest-jpeg.jpg"); | |||
await sourceCard.Attachments.Add(File.ReadAllBytes(jpeg), "smallest-jpeg.jpg"); | |||
var pdf = Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location), "Files/smallest-jpeg.jpg"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to change the client tests since they're associated with specific issues. Can you create a new test that uses the new code path, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright
/// <param name="name">A name for the sticker.</param> | ||
/// <param name="ct">(Optional) A cancellation token for async processing.</param> | ||
/// <returns>The <see cref="ISticker"/> generated by Trello.</returns> | ||
Task<ISticker> Add(string filePath, string name, CancellationToken ct = default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an override.
@@ -24,7 +24,7 @@ public IRestRequest Create(string endpoint, IDictionary<string, object> paramete | |||
if (parameter.Key == RestFile.ParameterKey) | |||
{ | |||
var rf = (RestFile)parameter.Value; | |||
request.AddFile(parameter.Key, rf.ContentBytes, rf.FileName); | |||
request.AddFile(parameter.Key, rf.FilePath, rf.FileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to check which attachment method we're using here. I don't want to completely replace the existing bytes method.
Manatee.Trello/Rest/WebApiClient.cs
Outdated
var fileStream = File.Open(request.FilePath, FileMode.Open); | ||
var fileInfo = new FileInfo(request.FilePath); | ||
formData.Add(new StreamContent(fileStream), "\"file\"", string.Format("\"{0}\"", "as" + fileInfo.Extension)); | ||
formData.Add(new StringContent("mimeType"), "image/png"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like to have some basic mime type checking rather than always assuming png.
if (request.FilePath != null) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I am verifying, whether the file is uploaded or byte array.
|
||
if (rf.FilePath != null) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason as above
public void AddFile(string key, string filePath, string fileName) | ||
{ | ||
FilePath = filePath; | ||
FileName = fileName; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, overloading the of AddFile method for assigning file location as well
I don't know what your editor is doing, but it keeps putting spaces in instead of tabs. The alignment is all off. Also, for the build, the card attachment test is failing. I'd like to also see a test that uses your method to attach a file. The |
|
Hi Greg,
#318 (comment) is working fine and is uploading all types of file correctly. However, the problem occurs when your tired to download the uploaded files (from trello board) specifically pdf, excel and document files etc. In addition when ones tries to view .pdf, .docx files will not be able to view it on trello board.
However, if the WebApiClient file method GetContent is modified like this
This code is still be able to upload all types of files, and the user will also be able to download file in original format, and can view it on the trello board as well.