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

[GSoC 2019] allow users to configure notification settings #391

Merged
merged 3 commits into from Aug 12, 2019

Conversation

@aquibbaig
Copy link
Collaborator

aquibbaig commented Jul 17, 2019

Allows users to configure Notification settings

@aquibbaig aquibbaig added the gsoc label Jul 17, 2019
@aquibbaig aquibbaig self-assigned this Jul 17, 2019
@aquibbaig aquibbaig changed the title [GSoC 2019] database changes for notification settings [GSoC 2019] allow users to configure notification settings Jul 17, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:notif-settings branch 3 times, most recently from 9a9d47a to ebb95bb Jul 17, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 22, 2019

@imphil The logic for this(change email subscription on click of the button) can be improved IMO. Don't you think it's a bit primitive?

@aquibbaig aquibbaig force-pushed the aquibbaig:notif-settings branch from ebb95bb to f1e7eb7 Jul 22, 2019
Copy link
Contributor

imphil left a comment

Thanks @aquibbaig for your work on this. Comments are in the code.

/**
* User Notification Settings
*
* @Route("/user/settings/notification-settings", name="librecores.user.settings.notification")

This comment has been minimized.

Copy link
@imphil

imphil Jul 29, 2019

Contributor

/user/settings/notification-settings -> /user/settings/notifications

(no need to have "settings" in the URL twice)

{% extends 'user/settings_base.html.twig' %}

{# Inform settings_base.html.twig about the page currently displayed #}
{% set settings_subpage = 'notification' %}

This comment has been minimized.

Copy link
@imphil

imphil Jul 29, 2019

Contributor

notification -> notifications

@@ -0,0 +1,30 @@
{#
Template for the user to edit notification settings
page path: /user/settings/notification-settings

This comment has been minimized.

Copy link
@imphil

imphil Jul 29, 2019

Contributor

Please fix that path (see below).


{% block settings_content %}
<h2>Notification Settings</h2>
<p> By default, all users get notifications from the Librecores community in the form of web notifications

This comment has been minimized.

Copy link
@imphil

imphil Jul 29, 2019

Contributor

Librecores -> LibreCores

<p> By default, all users get notifications from the Librecores community in the form of web notifications
on the website as well as email notifications to their registered email. The settings for each of these notifications
is down to personal preferences and can be edited in the panel down below</p>
<h4>Email Notifications</h4>

This comment has been minimized.

Copy link
@imphil

imphil Jul 29, 2019

Contributor

h2 is followed by h3, not h4

<form method="post" action="{{ path('librecores.user.settings.notification.email', {status: enStatus}) }}">
<button class="btn btn-success" type="submit">Turn On</button>
</form>
{% endif %}

This comment has been minimized.

Copy link
@imphil

imphil Jul 29, 2019

Contributor

Please make this a normal form with a checkbox to enable/disable notifications, and submit this form by clicking on a "submit" button. Use the symfony form component instead of manually writing a HTML form.

@aquibbaig aquibbaig force-pushed the aquibbaig:notif-settings branch 3 times, most recently from 6160743 to 11c3deb Jul 30, 2019
site/src/Entity/User.php Outdated Show resolved Hide resolved
@aquibbaig aquibbaig requested a review from agathver Jul 31, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:notif-settings branch from 11c3deb to aab8d04 Jul 31, 2019
Copy link
Contributor

imphil left a comment

Thanks @aquibbaig, looks like we're almost there.

@aquibbaig aquibbaig force-pushed the aquibbaig:notif-settings branch 4 times, most recently from 9d2ddcc to 87c2744 Aug 2, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 2, 2019

@imphil CheckBoxType is a fine option indeed. Looks like we're almost there now with this PR

@aquibbaig aquibbaig requested a review from imphil Aug 2, 2019
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Aug 7, 2019

This PR requires a rebase on top of master, but then it's ready to go.

@imphil
imphil approved these changes Aug 7, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:notif-settings branch from 87c2744 to f4fe918 Aug 8, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 8, 2019

@imphil Just rebased to update this branch

@aquibbaig aquibbaig force-pushed the aquibbaig:notif-settings branch from f4fe918 to 7a3f7b5 Aug 8, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 8, 2019

Needs a bit of cleansing now

Adds a tab to edit notification settings in the user profile and
also created it's corresponding view
@aquibbaig aquibbaig force-pushed the aquibbaig:notif-settings branch from 7a3f7b5 to 28ec012 Aug 8, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 8, 2019

@imphil I feel like this is ready to be merged now

@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 11, 2019

@imphil Do I add another commit here for applying these settings in the email consumer while sending out emails?

@imphil
imphil approved these changes Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.