Add NostrMedia.com as media uploader option #2571
Conversation
There was a problem hiding this comment.
Hello @Spl0itable, thank you for your PR! Code changes look mostly ok, but there are some things that need to be addressed:
[❌ ] I have tested the changes in this PR
Test report
N/A -- not a mobile app dev and this change seems small enough to not warrant a full blown emulator test. NostrMeida.com API endpoint supports NIP-98 and NIP-96 as well as Blossom. So, if the POST and/or PUT requests are structured to spec, then there shouldn't be an issue with passing uploads or having the uploaded file URL returned in response.
Even though Damus and nostrmedia.com communicate using the same protocols, it's important to try out the new media uploader option to see if there is any unexpected behavior.
We don't need a full blown test, but it would help us to understand how you tested the changes to ensure they work (e.g. Have you tried uploading images into a new post using this new option to see if it works?).
[✅] My PR is either small, or I have split it into smaller logical commits that are easier to review
The PR currently contains two commits with the same message. Can we please squash commits?
[❌ ] I have added the signoff line to all my commits. See Signing off your work
Please add the signoff line as per our contribution guidelines. Please read Signing off your work
[✅ ] I have added appropriate changelog entries for the changes in this PR. See Adding changelog entries
This PR introduces a user-facing change, and the changelog entry is missing. Please see Adding changelog entries
[❌ ] I have added appropriate Closes: or Fixes: tags in the commit messages wherever applicable, or made sure those are not needed. See Submitting patches
This item is not needed for this PR.
On top of the above items, I also added one inline code comment.
Please let me know if you have any questions. Thank you!
| @@ -11,6 +11,7 @@ enum MediaUploader: String, CaseIterable, Identifiable, StringCodable { | |||
| var id: String { self.rawValue } | |||
| case nostrBuild | |||
| case nostrcheck | |||
| case nostrMedia // New case for nostrMedia | |||
There was a problem hiding this comment.
This code comment can be removed.
|
@Spl0itable, to help illustrate our guidelines with a reference, here is one recent example of a pull request that follows the guidelines: #2570 Please let me know if you have any questions, I would be happy to clarify them. Thanks! |
|
Thanks @danieldaquino I did include the changelog entry. No worries, I'll test in emulator to confirm and correct the other requirements. Cheers |
@Spl0itable, ah I see what happened, I think there was a misunderstanding. I just noticed that you indeed have the changelog entry on your PR message, but we actually need them to be on the commit message itself. I apologize if that part of the guidelines wasn't clear. The reason we need them on the commit message is so that a script can pick them up and help us save us time when writing the changelog for users after each release. Your changelog line looks good, so we just need to move it to the commit message. You can try running Please let me know if there is anything else I can help with! |
Thank you! I appreciate it 🙏 |
Summary
I do not believe a SoB is necessary for this small merge request? I'm also not a Swift dev nor mobile app dev, but the code's logic seemed fairly easy to replicate in order to add edits. Please correct if not accurate and if there are other required file edits necessary. However, I could not find the "Appearance" UI that controlled the drop-down menu in user settings in order to add an option for NostrMedia.com.
Interestingly, I couldn't find the file that controls where "nostrimg.com" is even listed, which appears like it should be deprecated as "nostrimg.com" is no longer a functioning website. Yet, the option for "nostrimg.com" still exists even in the current production v1.10.1 build released in App Store. (see screenshots below)
Changelog-Added: media uploader support for NostrMedia.com
Checklist
Closes:orFixes:tags in the commit messages wherever applicable, or made sure those are not needed. See Submitting patchesTest report
N/A -- not a mobile app dev and this change seems small enough to not warrant a full blown emulator test. NostrMeida.com API endpoint supports NIP-98 and NIP-96 as well as Blossom. So, if the POST and/or PUT requests are structured to spec, then there shouldn't be an issue with passing uploads or having the uploaded file URL returned in response.
Other notes
Please let me know if there's anything else required in order to get NostrMedia.com added as a media upload provider. Thank you!