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

Add status upload file feature #5840

Closed
wants to merge 3 commits into from

Conversation

iamonuwa
Copy link
Contributor

@iamonuwa iamonuwa commented Jan 19, 2020

Description

Allow users to upload images as part of status

Refers/Fixes

Fixes #5832

Testing

Check the user profile, try uploading an image alongside the status or check the townsquare status box

@iamonuwa
Copy link
Contributor Author

@octavioamu @owocki @danlipert, how do you want this to look like? if that image is flagged by >3 people or an admin, then it is hidden + the user is not able to upload images anymore

AFAIK, the flagging haapens on each of the updates

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

remove migrations and recreate as one neat migration + left a few other comments
Could you add a recording of the PR

@iamonuwa
Copy link
Contributor Author

screen-capture-9

@codecov
Copy link

codecov bot commented Jan 20, 2020

Codecov Report

Merging #5840 into master will decrease coverage by <.01%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5840      +/-   ##
==========================================
- Coverage   29.55%   29.54%   -0.01%     
==========================================
  Files         266      266              
  Lines       22653    22661       +8     
  Branches     3289     3291       +2     
==========================================
+ Hits         6694     6696       +2     
- Misses      15682    15688       +6     
  Partials      277      277
Impacted Files Coverage Δ
app/dashboard/models.py 55.73% <100%> (+0.01%) ⬆️
app/retail/views.py 24.12% <14.28%> (-0.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8b5a1e...9d6d408. Read the comment docs.

@iamonuwa
Copy link
Contributor Author

remove migrations and recreate as one neat migration + left a few other comments
Could you add a recording of the PR

Removing the migrations and creating again yields same result

@androolloyd
Copy link
Contributor

remove migrations and recreate as one neat migration + left a few other comments
Could you add a recording of the PR

Removing the migrations and creating again yields same result

The issue is that a merge had to be done which makes for a non linear path on migrations.

If you remove the migrations, rebase onto master and recreate the migrations you should get a single file created.

If not ping me on chat and we can figure it out, I have had to go through this a number of times already.

@danlipert
Copy link
Contributor

@iamonuwa do the images get shown anywhere in the feed or will that be a separate PR? I'd like to see that functionality together

@iamonuwa
Copy link
Contributor Author

Separate PR, yes. The spec of this PR was to allow status upload

@owocki
Copy link
Contributor

owocki commented Jan 21, 2020

what does the image look like when its been posted to the feed? does it look good on mobile/desktop/tablet?

@iamonuwa
Copy link
Contributor Author

@owocki, it uploads to s3

@iamonuwa
Copy link
Contributor Author

@owocki, this pr uploads images with the status but doesn't display I.

@owocki
Copy link
Contributor

owocki commented Jan 22, 2020

and attached to the status update post

ah ok when i said this i meant that it should be displayed :)

@iamonuwa
Copy link
Contributor Author

and attached to the status update post

ah ok when i said this i meant that it should be displayed :)

Am going to show image if status has an image attached.

@owocki
Copy link
Contributor

owocki commented Mar 4, 2020

how goes? we get this feature requested regularly.. can we get it mergeable?

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Mar 4, 2020

how goes? we get this feature requested regularly.. can we get it mergeable?

waiting for reviews

@danlipert
Copy link
Contributor

@iamonuwa does this PR now show the uploaded image or is that coming in a followup PR?

@iamonuwa
Copy link
Contributor Author

Follow up PR. This weekend. Everywhere is hot right now with the pandemic.

@danlipert
Copy link
Contributor

Follow up PR. This weekend. Everywhere is hot right now with the pandemic.

no worries man - stay safe

@thelostone-mc
Copy link
Member

@iamonuwa looks like this PR has gotten stale! Could you throw in a fresh PR whenever you get around to this & we can get this in ?
Closing it out for now (reopen it -> if you'll be working on it / got a solution already ready based on the feedback comments)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants