-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
Resolves #4689 - Update seeds: mark users invitation_status
as "accepted"
#4705
base: main
Are you sure you want to change the base?
Conversation
invitation_status
as "accepted"invitation_status
as "accepted"
@@ -833,3 +833,7 @@ def seed_quantity(item_name, organization, storage_location, quantity) | |||
] | |||
) | |||
TransferCreateService.call(transfer) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓Question: For this file I see some common comments sections, should I add some comments to explain why the users' invitation_status
is being changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't hurt!
@@ -111,6 +111,12 @@ def invitation_status | |||
"invited" if invitation_sent_at.present? | |||
end | |||
|
|||
def mark_invitation_status_as_accepted! | |||
self.invitation_sent_at = Time.current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓Question: Should I be concerned about the other related invitation attributes? Like, for example, the attributes invitation_created_at
, or invitation_limit
, or invitation_token
, etc…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope.
@@ -111,6 +111,12 @@ def invitation_status | |||
"invited" if invitation_sent_at.present? | |||
end | |||
|
|||
def mark_invitation_status_as_accepted! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓Question: Another possible solution would be to implement this inside the seeds file instead of the User
model class. I find it clearer this way, but the User
class is at risk of becoming a "monster class," and this addition might encourage others to add even more complexity to it. Do you think it's okay as it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I would move this method to the seeds file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I would move this method to the seeds file.
invitation_status
as "accepted"invitation_status
as "accepted"
@@ -111,6 +111,12 @@ def invitation_status | |||
"invited" if invitation_sent_at.present? | |||
end | |||
|
|||
def mark_invitation_status_as_accepted! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I would move this method to the seeds file.
@@ -111,6 +111,12 @@ def invitation_status | |||
"invited" if invitation_sent_at.present? | |||
end | |||
|
|||
def mark_invitation_status_as_accepted! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I would move this method to the seeds file.
@@ -111,6 +111,12 @@ def invitation_status | |||
"invited" if invitation_sent_at.present? | |||
end | |||
|
|||
def mark_invitation_status_as_accepted! | |||
self.invitation_sent_at = Time.current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope.
@@ -833,3 +833,7 @@ def seed_quantity(item_name, organization, storage_location, quantity) | |||
] | |||
) | |||
TransferCreateService.call(transfer) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't hurt!
Resolves #4689
Description
When we run the
bin/setup
script, it triggers various processes, including the Rails commandbin/rails db:reset
. This command is responsible for dropping the database and setting it up again (which actually creates the database, loads the schema, and initializes it with seed data). The seed data is located indb/seeds.rb
, and this file should contain all the necessary record creation to populate the database with demo values.We discovered that when a partner service attempts to invite a user created during the
bin/setup
script, the error "nil can't be converted to a Time value" is raised. This happens because those users don't have the invitation attributes set.This PR adds extra lines to the
db/seeds.rb
file to mark all created users'invitation_status
asaccepted
. Specifically, it sets theinvitation_sent_at
andinvitation_accepted_at
attributes for each user. I decided to modify only these values after reviewing theUser#invitation_status
method and this related test.Regarding the question left on the issue
| It does send the email -- it's just failing on displaying?
I'm not entirely sure, but based on the logs, there was a background job failure after I sent the user invite that I noticed. Should I be concerned about this?
Type of change
How Has This Been Tested?
I followed the steps to reproduce mentioned in the issue:
bin/setup
bin/start
I ran the test scenario that I added with
bundle exec rspec spec/models/user_spec.rb:134
I also ran the
bin/lint
commandI ran the entire test suite with
bundle exec rspec
Screenshots
(Before) On the "Manage users" partner page, after clicking on "Invite User"
(After) On the "Manage users" partner page, after clicking on "Invite User"