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

code: Notifications #3221

Merged
merged 129 commits into from
Jan 16, 2019
Merged

code: Notifications #3221

merged 129 commits into from
Jan 16, 2019

Conversation

octavioamu
Copy link
Contributor

@octavioamu octavioamu commented Dec 14, 2018

Description
Checklist
Affected core subsystem(s)
Refers/Fixes

Refs: #2404 #3083

Testing and Sign-off
Contributor
  • Read and followed the Contributor Guidelines
  • Tested all changes locally
  • Verified existing functionality
  • Ran make test and everything passed!
Reviewer
  • Affirm contributor guidelines have been followed and requested changes made
  • CI tests and linting pass
  • No conflicts (migrations, files, etc)
  • Regression tested against staging
Funder
  • Validated requested changes were made to specification
  • Bounty payout released to the contributor

@octavioamu octavioamu changed the title Notifications [WIP] DONT MERGE Notifications Jan 9, 2019
app/inbox/signals.py Outdated Show resolved Hide resolved
app/inbox/signals.py Show resolved Hide resolved
app/inbox/signals.py Show resolved Hide resolved
app/inbox/signals.py Show resolved Hide resolved
app/inbox/signals.py Show resolved Hide resolved
app/inbox/signals.py Show resolved Hide resolved
app/inbox/signals.py Show resolved Hide resolved
app/inbox/signals.py Show resolved Hide resolved
app/inbox/signals.py Show resolved Hide resolved
app/inbox/signals.py Show resolved Hide resolved
@owocki
Copy link
Contributor

owocki commented Jan 14, 2019

test notes:

  • start / submit work works really well
  • when i send a new tip to a user, there is no notification
  • when i send a new kudos to a user, there is no notification

@@ -69,12 +77,20 @@ h4,
position: fixed;
top: 0;
left: 0;
/* right: 0; */
Copy link
Contributor

Choose a reason for hiding this comment

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

dead code; remove?

let numPages = '';
let numNotifications = '';

Vue.mixin({
Copy link
Contributor

Choose a reason for hiding this comment

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

excited to learn more about Vue -- itd becool to have a brownbag from you hearing how we can leverage Vue more for our codebase!

cc @SaptakS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that explorer and bounty details are great candidates to be in Vue, will facilitate a lot the states changes from the json request.

('new_milestone', 'New Milestone'),
('update_milestone', 'Updated Milestone'),
('new_kudos', 'New Kudos'),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

@usmanmuhd can you confirm that all of these notification types have been integrated?

Copy link
Contributor

Choose a reason for hiding this comment

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

for example, i dont see killed_grant listed anywhere else in the codebase

('bounty_removed_by_funder', 'Removed from Bounty by Funder'),
('new_crowdfund', 'New Crowdfund Contribution'),
('new_grant', 'New Grant'),
('update_grant', 'Updated Grant'),
Copy link
Contributor

Choose a reason for hiding this comment

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

update_grant not implemented either..

can you do a more exhaustive audit of these... we need to know which are implemented and which arent'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I said to @usmanmuhd to do the ones he could do, and we can see later the others. I mean is something we can be gradually implementing I believe.

@thelostone-mc thelostone-mc changed the title [WIP] DONT MERGE Notifications code: Notifications Jan 16, 2019
Copy link
Contributor

@SaptakS SaptakS left a comment

Choose a reason for hiding this comment

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

LGTM

@SaptakS SaptakS merged commit 15e2a4a into master Jan 16, 2019
@thelostone-mc thelostone-mc deleted the vue-notifications branch March 27, 2019 18:23
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.

7 participants