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

Invitation for booth responsibles #2535

Merged
merged 4 commits into from
Jul 21, 2019

Conversation

rahul2240
Copy link
Member

@rahul2240 rahul2240 commented Jun 9, 2019

Related to #2527

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the upstream master branch.
  • The tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works(if appropriate).

Need

Whenever a conference is organized, people submit booth request for that conference. It is possible to add responsibles to booth but only registered users can be added as responsibles.

With this PR, booth submitter will be able to invite unregistered users to join the app and handle the booth. Making it easier for the submitter to add users as responsibles only one time rather than waiting for users to first join and then adding them as responsibles.

Workflow

  1. User submits a booth request.
  2. This invitation field will be available in new booth and edit booth form for submitters.
  3. User can enter any number of emails that are needed to be invited.
  4. If user with that email already exists in user db then he/she will become booth responsible.
  5. If user with that email doesn't exist in user db then that user will be created and an invitation mail will be sent to the user with an invitation token to join the app.
  6. Once the user joins the app, they will be able to access the booth.

new booth view
ready

mail
accept1

Registartion page after accepting link
accept2

After registration my booth option available
accept3

columns added to User
db

@codecov
Copy link

codecov bot commented Jun 9, 2019

Codecov Report

Merging #2535 into GSoC19 will decrease coverage by 6.64%.
The diff coverage is 26.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           GSoC19    #2535      +/-   ##
==========================================
- Coverage   82.58%   75.94%   -6.65%     
==========================================
  Files         146      146              
  Lines        4767     4847      +80     
==========================================
- Hits         3937     3681     -256     
- Misses        830     1166     +336
Impacted Files Coverage Δ
app/models/user.rb 95% <100%> (+0.03%) ⬆️
app/models/booth.rb 88% <100%> (+0.24%) ⬆️
app/controllers/application_controller.rb 77.77% <100%> (+2.02%) ⬆️
app/controllers/booths_controller.rb 50% <12.5%> (-12%) ⬇️
app/controllers/admin/booths_controller.rb 50% <12.5%> (-8.34%) ⬇️
app/controllers/tickets_controller.rb 0% <0%> (-90%) ⬇️
app/controllers/ticket_purchases_controller.rb 0% <0%> (-77.28%) ⬇️
app/controllers/admin/volunteers_controller.rb 0% <0%> (-66.67%) ⬇️
app/helpers/versions_helper.rb 27.08% <0%> (-56.25%) ⬇️
app/controllers/payments_controller.rb 0% <0%> (-51.86%) ⬇️
... and 24 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 6140688...98a57e0. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 9, 2019

Codecov Report

Merging #2535 into GSoC19 will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           GSoC19    #2535     +/-   ##
=========================================
+ Coverage   82.21%   82.31%   +0.1%     
=========================================
  Files         146      147      +1     
  Lines        4773     4795     +22     
=========================================
+ Hits         3924     3947     +23     
+ Misses        849      848      -1
Impacted Files Coverage Δ
app/models/user.rb 94.96% <ø> (ø) ⬆️
app/controllers/booths_controller.rb 64.15% <100%> (+2.15%) ⬆️
app/controllers/concerns/invitation.rb 100% <100%> (ø)
app/models/booth.rb 88% <100%> (+0.24%) ⬆️
app/controllers/admin/booths_controller.rb 60% <100%> (+1.66%) ⬆️
app/controllers/application_controller.rb 77.77% <100%> (+2.02%) ⬆️
app/controllers/commercials_controller.rb 96.42% <0%> (+3.57%) ⬆️

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 12f10c7...29c15c5. Read the comment docs.

@rahul2240 rahul2240 force-pushed the booth_invitation branch 3 times, most recently from 6a122c5 to f094553 Compare June 9, 2019 18:59
Gemfile Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/controllers/admin/booths_controller.rb Outdated Show resolved Hide resolved
spec/controllers/admin/booths_controller_spec.rb Outdated Show resolved Hide resolved
@Ana06
Copy link
Member

Ana06 commented Jun 12, 2019

@rahul2240 you also need to fix the lint offenses and tests (in general it is a good practice to run the test locally before sending the PR 😉)

@rahul2240
Copy link
Member Author

@rahul2240 you also need to fix the lint offenses and tests (in general it is a good practice to run the test locally before sending the PR )

@Ana06 I have 1 lint only for 105 lines used whose limit is 100, so can you tell me what i should do and i dont know what error is breaking the other tests as it is something related to Selenium::WebDriver::Error::UnknownError:
unknown error: Chrome failed to start: exited abnormally

so i send a PR to ask about it.

@Ana06
Copy link
Member

Ana06 commented Jun 13, 2019

@rahul2240

so i send a PR to ask about it.

but you didn't ask 🤣 It is fine. Let's check if I can help you. 😉

@Ana06 I have 1 lint only for 105 lines used whose limit is 100, so can you tell me what i should do

When a class gets to big, it is normally because there are things on it that doesn't belong there. So, you need to split things out (moving model related logic to the models, using concerns, introduce new controllers, etc.). invite is currently in the User model, but it is not necessarily related to the user. It is maybe time to put everything related with the invitation (in both user model and booth controller) out and together.

and i dont know what error is breaking the other tests as it is something related to Selenium::WebDriver::Error::UnknownError:
unknown error: Chrome failed to start: exited abnormally

This is not related to your PR, it is failing in other PRs as well. We need to fix it, but not as part of this PR.

@rahul2240
Copy link
Member Author

@Ana06 I forgot to ask :P, Thanks for the help, For the upcoming work that I mentioned I am creating an invitation controller (for admins only) and model also to handle things related to model. But for booths I thought it would be better if I use invitation in booths controller itself as it is the only feature that user going to have, related to invitation.

@differentreality
Copy link
Contributor

@rahul2240 let's discuss a bit what you are trying to accomplish. What's the plan?
I'd expect you to provide the workflow you are trying to implement in the PR description or the issue relevant to this particular feature and PR.

The following is more of a conversation starter than a final plan.

  • User submits booth request
  • User invites 2 people to become booth responsibles
  • Devise invitable creates 2 new user records, and sends invitation
  • The new user accounts are already associated with the booth request
    (but in the end we should only associate real users with a Booth, at least upon acceptance)

@rahul2240 rahul2240 force-pushed the booth_invitation branch 5 times, most recently from 3873e20 to b17738f Compare June 16, 2019 19:44
@rahul2240
Copy link
Member Author

@differentreality @Ana06 I have removed some commits to make this PR small and related to 1 thing only. I will create other PR for an unsubscribing link in the invitation email and customized invitation message.
Please review this PR again 😄

@rahul2240
Copy link
Member Author

rahul2240 commented Jun 16, 2019

@rahul2240 let's discuss a bit what you are trying to accomplish. What's the plan?
I'd expect you to provide the workflow you are trying to implement in the PR description or the issue relevant to this particular feature and PR.

The following is more of a conversation starter than a final plan.

  • User submits booth request
  • User invites 2 people to become booth responsibles
  • Devise invitable creates 2 new user records, and sends invitation
  • The new user accounts are already associated with the booth request
    (but in the end we should only associate real users with a Booth, at least upon acceptance)

@differentreality User can invite any number of people. I can add minItem: 2 at the form to only allow 2 users to invite at a time.
I will work on associating only confirmed users with roles, booths, conferences in separate PR.

@@ -9,6 +9,8 @@ class ApplicationController < ActionController::Base
# Ensure every controller authorizes resource or skips authorization (skip_authorization_check)
check_authorization unless: :devise_controller?

before_action :devise_configure_permitted_parameters, if: :devise_controller?
Copy link
Member Author

Choose a reason for hiding this comment

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

devise_parameter_sanitizer.permit(:accept_invitation, keys: [:username])

used to add username field in the registration page for invited users otherwise only password and password confirmation fields were present.

@differentreality
Copy link
Contributor

  1. Once the users will register to the app then they will be able to edit the booth for which they were invited.

How is this going to happen exactly?

In your steps you mention that you will create an unconfirmed user. How is this user going to be related to a booth?

@rahul2240
Copy link
Member Author

rahul2240 commented Jun 20, 2019

@differentreality unconfirmed user's id will be used to associate that user with the booth using

BoothRequest.create(booth_id: @booth.id, user_id: created_user.id,
                              role: 'responsible')

If a user registers then there is no problem.
If he doesn't, then invited user will not be visible to the booth owner in the responsibles field in edit booth page. This way booth owner will know that he needs other booth responsibles or reinvite the same user.
Only confirmed users are visible in the responsibles field for new or edit booth.


emails_array.each do |email|
new_user = User.find_by(email: email)
if new_user.nil? || !new_user.confirmed?
Copy link
Contributor

Choose a reason for hiding this comment

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

!new_user.confirmed? that's not bulletproof. It's possible that we have unconfirmed users (when using) but the users are actually able to log in and don't require invitation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@differentreality we can add user's "last_sign_in_at", if it is nil then we can send invitations again, is that okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is already a user record, why do we need to check anything else?

I mean there are a few ways we can go with this approach and make the feature more useful and meaningful, but what is currently being implemented is pretty basic, so I don't think we should add any more functionality than what is absolutely necessary, unless you plan on documenting (and adding tests) for the more advanced scenarios.

Copy link
Member Author

Choose a reason for hiding this comment

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

@differentreality alright, for now, I will keep it simple but will work on improving it later in other PR.

app/helpers/invite_helper.rb Outdated Show resolved Hide resolved
@rahul2240 rahul2240 force-pushed the booth_invitation branch 11 times, most recently from 571fd76 to a1a7857 Compare July 20, 2019 09:36
@differentreality
Copy link
Contributor

@rahul2240 please rebase

The tests are not very thorough here :/ We should revisit them soon.

@rahul2240 rahul2240 force-pushed the booth_invitation branch 5 times, most recently from ef9ae35 to 0500a24 Compare July 21, 2019 11:15
@rahul2240
Copy link
Member Author

rahul2240 commented Jul 21, 2019

The tests are not very thorough here :/ We should revisit them soon.

Sure.

It is added so that user can enter email ids of other users, who they want to invite to become booth responsible.
email validation and input as tags is added using selectize
Devise invitable gem is required to invite users.
User will enter email of users to invite to become booth responsible, which will create a user in User db with that email and a new booth request will be created.
@differentreality
Copy link
Contributor

Only confirmed users are visible in the responsibles field for new or edit booth.

Where do you see that?

I was talking about active users
while adding booth responsibles in the booth, only active users are available in the list due to the condition
https://github.com/openSUSE/osem/blob/master/app/helpers/application_helper.rb#L135

This isn't right though! Active users used to be something completely different, and now they are the ones who logged in during the last 3 months.
This is the commit that changed the definition 516e53d

We need to actually get all active (as in not disabled) users for our selection field. Do you want to submit a PR for that in master, or should I?

@differentreality
Copy link
Contributor

OK let's get this in as a starting point, and work on it a bit more later on to add more possibilities

  • see who has been invited
  • trigger re-invitation
  • maybe even undo a pending invitation (or actually remove the user as booth responsible?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GSoC GSoC 19 related issues/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants