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

Show dropdown when user is about to tag a person using @ #5839

Merged
merged 5 commits into from
Jan 24, 2020

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Jan 19, 2020

Description

Show dropdown of a list of users when a @ is detected in the textarea.
image

Refers/Fixes

Fixes #5823.

Testing

@codecov
Copy link

codecov bot commented Jan 19, 2020

Codecov Report

Merging #5839 into master will decrease coverage by 0.09%.
The diff coverage is 21.87%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #5839     +/-   ##
=========================================
- Coverage   29.55%   29.45%   -0.1%     
=========================================
  Files         266      265      -1     
  Lines       22653    22862    +209     
  Branches     3289     3320     +31     
=========================================
+ Hits         6694     6734     +40     
- Misses      15682    15854    +172     
+ Partials      277      274      -3
Impacted Files Coverage Δ
app/retail/emails.py 23.27% <22.22%> (-0.02%) ⬇️
app/townsquare/views.py 10.81% <25%> (-2.06%) ⬇️
app/retail/views.py 23.13% <66.66%> (-1.17%) ⬇️
app/marketing/mails.py 13.02% <8.33%> (-0.07%) ⬇️
app/townsquare/admin.py 41.53% <0%> (-58.47%) ⬇️
app/townsquare/tasks.py 46.42% <0%> (-8.58%) ⬇️
app/dashboard/utils.py 37.47% <0%> (-3.31%) ⬇️
app/grants/admin.py 47.24% <0%> (-1.83%) ⬇️
... and 10 more

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...5e30e3e. Read the comment docs.

@KiChjang
Copy link
Contributor Author

KiChjang commented Jan 21, 2020

I'm unsure if I need a new migration for this -- my guess is that I don't, because python defaults missing email preferences to a false value.

I'm also not quite understanding why Travis CI is failing, perhaps it's an infrastructure issue?


<p>

<a href="{{psot.profile.url}}">@{{post.profile.handle}}</a> mentioned you in a post. This is what the post looks like:
Copy link
Contributor

Choose a reason for hiding this comment

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

typo here

@@ -1250,6 +1264,13 @@ def comment(request):
return HttpResponse(response_html)


@staff_member_required
def mention(request):
Copy link
Contributor

Choose a reason for hiding this comment

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

is this for testing? Is there a URL that points to this view?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but I figured that this might be a function that can be called by a staff member to manually trigger the email, since other kinds of notifications also have a similar method (look above and below).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see - normally we hook these views into the router behind our admin endpoint like this: https://github.com/gitcoinco/web/blob/master/app/app/urls.py#L486
That way our design team can check out what the email looks like - can you go ahead and hook this view into the router? thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

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.

codewise lgtm

@danlipert danlipert merged commit 58826bd into gitcoinco:master Jan 24, 2020
@KiChjang KiChjang deleted the username-tag branch January 27, 2020 02:52
@owocki
Copy link
Contributor

owocki commented Feb 5, 2020

guys.. this needs testing next time https://bits.owocki.com/bLuGWxPg
58826bd#commitcomment-37065669

@owocki
Copy link
Contributor

owocki commented Feb 5, 2020

antoher one https://bits.owocki.com/v1umZLXn

this PR had no less than 3 errors in different places

owocki added a commit that referenced this pull request Feb 5, 2020
@owocki
Copy link
Contributor

owocki commented Feb 5, 2020

here is my fix 75f609f

@KiChjang
Copy link
Contributor Author

KiChjang commented Feb 5, 2020

@owocki Sorry, I didn't quite understand the backend architecture and was hoping that my mistakes would be caught during code review. Is there any documentation I can read in order to familiarize myself with it?

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.

add ability to tag people in status updates, comments, and wall posts
4 participants