Feat/register user last login resolves #99

[FranEnLaNube]

There are different ways to solve this.

• Using events or listeners.
• Adding a last_login_at field to users table. (I've chosen the this one.

Also there are two ways to do it.

• Modifying an existing migration
• Creating a specific one.

After talk about this in the meeting we decided to do a specific one because the application is already in production and we want to use the benefits of migrations history.

These are the articles I've read to support this task.

• Cursodesarrolloweb This one was particularly useful and clear for me to do a specific migration
• Pleets Blog Here we have interesting info about migrations and mention that in production is recommended to do specific migrations
• Styde.net Another interesting article about migrations, particularly to help making model relation, but also recommend to do a specific one
• Laracast Here we have a good and clear answer to an user to is trying to avoid using an event to save this information
• SOF question where explain easily how to make a new specific migration
• qastack.mx In this one they explain similarly but recommend to use the flag --table=users to ensure the field is added to users table in this case, and also is gonna create up() and down() methods automatically.
• SOF I've used to change timezone
• Laravel docs about migrations
• Laravel docs about modifiers

Changes

• Added a new specific migration: 2023_11_13_160349_add_last_login_at_to_users.php

  • up() Ads a column last_login_at , nullable, after the role one.
    • Modifiers, like ->nullable() are used to add characteristics to a field.
  • down() To revert this migration by deleting this column.
  • Laravel docs about migrations

• User.php To be able to use update() in the controller I've added last_login_at field to $fillable [ ]

  • Otherwise we need to do $user->last_login_at = now(); $user->save();

• AuthController.php -At login() I added $user->update(['last_login_at' => now()]); , the field is NULL when a new user is created. -Also could use => Carbon::now() or => new DateTime(), first one it's just the long way and second is another class which to the same. Like it's said here

  • In a future, we'll need to take care if we use a 'remember me' option which skip the log in, it could bring problems.

• AnnotationsUser.php Add last_login_at field to user swagger annotations.

• config/app.php Set new timezone following .env variable, we'll need to set this in the .env file with a variable like this one: APP_TIMEZONE='Europe/Madrid'

• LoginTest.php Two more tests to check if the field is updated when a user is logged and if is not when login fails

  • I couldn't log the user out, we are missing that route.
  • I think I didn't follow Ruben's recommendations about assert in this PR #55
  • I still have same fails that are commented in Issue #97

    Others PR related with this issue

    Testing

• PR #50 and PR #55 about login testing. https://github.com/IT-Academy-BCN/ita-landing-backend/pull/50 y https://github.com/IT-Academy-BCN/ita-landing-backend/pull/55

  Migrations

• PR #49 modification of create_users_table https://github.com/IT-Academy-BCN/ita-landing-backend/pull/49

• PR #56 modification of create_faqs_table and create_apps_table migrations. https://github.com/IT-Academy-BCN/ita-landing-backend/pull/55

• PR #62 modification of retet_tokens_table migration. https://github.com/IT-Academy-BCN/ita-landing-backend/pull/62

• PR #73 modification of create_codes_table migration. https://github.com/IT-Academy-BCN/ita-landing-backend/pull/73

• PR #89 make add-column-to-apps migration. https://github.com/IT-Academy-BCN/ita-landing-backend/pull/89

Comments

• Later consider using "squashing migrations"

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.7% Duplication

[FranEnLaNube]

I've made two modifications to solve sonarcloud comments.

• Shortened the comment in app.php to follow PER rules.
• Modified test by using Factory class to avoid repetition of the user data arrays.
  • Regards to the repetition of string passport:install in \Artisan::call('passport:install');: I've tried to move it to the beginning of the class with a setup or setupBeforeClass method. But, it couldn't guarantee to be ran when a specific test is called.
  • About the repetition of string "invalid credentials": I've tried to use 'message' => __('auth.failed') but, as we have personalized JSON response in our AuthController they didn't match.

[CloudSalander]

Need @androsrivas check when its possible.

First of all, @franenlanube THANK YOU for the huge explanations, that is a very generous use of a Pull Request.

About the feature:

• Not necessary to put de id of the task in the name of the branch. We prefefer "feature" than "feat".
• I find that now you did very little commits. Basically, commit per file change. Thats a little extreme. It's more about doing versions about small logical parts of a feature, not for every file change. I'm sure you will find your balance in that sense.
• Finally, and probably most important, your feature its working.

Congratulations!

[androsrivas]

Great job @franenlanube ! Last Sunday I noticed that there was no Logout method in AuthController. Luckily, that can be fixed quickly! Thank you for such a thorough PR!!