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

User settings page #2828

Closed
wants to merge 5 commits into from
Closed

Conversation

grvsachdeva
Copy link
Member

Fixes #2796

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • PR body includes fixes #0000-style reference to original issue #
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

@ghost ghost assigned grvsachdeva Jun 14, 2018
@ghost ghost added the in progress label Jun 14, 2018
@grvsachdeva grvsachdeva changed the title Setting page User settings page Jun 14, 2018
@plotsbot
Copy link
Collaborator

plotsbot commented Jun 14, 2018

1 Warning
⚠️ It looks like you merged from master in this pull request. Please rebase to get rid of the merge commits – you may want to rewind the master branch and rebase instead of merging in from master, which can cause problems when accepting new code!
1 Message
📖 @Gauravano Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

@jywarren
Copy link
Member

I think this error is intermittent and unrelated -- there's an issue at #2824 ?

Restarted it!

@jywarren
Copy link
Member

Great, just a CodeClimate thing - want to approve it?

@grvsachdeva
Copy link
Member Author

hi @jywarren looks you have to Class UsersController has 21 methods (exceeds 20 allowed) . Thanks.

@ViditChitkara
Copy link
Member

Great work @Gauravano !! However, we need to come up with a minimum number of tags which need to be added for which a cron job is to be made in scheduler.rb . e.g digest:daily, digest:monthly, digest:daily and so on..Could you have a look at #2772 . I have used def customize_digest(type) method to add and remove these tags. You might want to use them in the controllers, once it gets merged. @jywarren any ideas??

@grvsachdeva
Copy link
Member Author

hi @ViditChitkara , I will adjust my view according to your controller function after your PR will be merged. Thanks.

@ViditChitkara
Copy link
Member

Cool 👍

@jywarren
Copy link
Member

OK, i'll approve CodeClimate then. I think it's OK. It'll simplify once we get rid of DrupalUsers - #956

Great teamwork folks, just ping me when you're ready!

@grvsachdeva
Copy link
Member Author

Hey @jywarren , @ViditChitkara and I was just discussing whether a user should be able to select only one type of digest from weekly digest and daily digest or not. what do you think about this?

@jywarren
Copy link
Member

Ah, like not both? Is there an efficiency or code simplicity reason to not allow both? If there isn't, i guess we can let people get a daily AND weekly digests... odd but maybe someone would like it?

@ViditChitkara
Copy link
Member

I thought of it a bit and came to a conclusion that, if we allow both, every week user would get 2 digest mails which is kind of inappropriate (and daily mails too). However, if a user opts for daily digest, he we automatically get weekly mails. While if a user chooses weekly, then most probably he/she wants only weekly mails and not daily. Makes sense??

@jywarren
Copy link
Member

Looks like #2772 is done although we are still trying to figure it out (any help appreciated) as it's not sending in production. But are we ready to merge this PR and pick up the settings again? Thanks!

@grvsachdeva
Copy link
Member Author

As we don't have digest template or any final preparation. I think I should close this one for now. I will make the settings page based on current settings in #2985 .Thanks!

@ghost ghost removed the in progress label Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PLANNING ISSUE: Subscription Settings
4 participants