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

Srodif/create deleted at column #565

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

srodif
Copy link

@srodif srodif commented Jul 30, 2024

Issue link / number:

#530

What changes did you make?

Added deletedAt column at user table and the database migration.

Why did you make the changes?

Preference of the team for the deletion date data to have a separate column.

Did you run tests?

Yes

Please give any feedback to make this PR more complete, thank you in advance for your time!
Requested as reviewers in issue:
@eleanorreem
@annarhughes

Copy link
Member

@kyleecodes kyleecodes left a comment

Choose a reason for hiding this comment

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

Hi @srodif, thanks for this PR and patience as our devs were out of office.
This looks almost good to merge, just needs tweaked to pass tests.
In addition to updating the mockData as you did, the userEntity object in src/partner-admin/partner-admin-auth.guard.spec.ts also needs updated with deleteAt in order to pass tests.

Were you able to run tests locally?

@srodif
Copy link
Author

srodif commented Aug 15, 2024

Hello @kyleecodes !
Thank for your time and your review on this! I updated the userEntity object in src/partner-admin/partner-admin-auth.guard.spec.ts too. I also was able to run the tests locally.

If you notice anything else that needs to change or that is missing, please let me know.

@srodif srodif requested a review from kyleecodes August 20, 2024 17:43
Copy link
Member

@kyleecodes kyleecodes left a comment

Choose a reason for hiding this comment

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

Hi @srodif, thanks for your patience as I was out of office.!

  1. This migration is close, but incomplete. It needs to set the user.deletedAt column to the date of user.updatedAt where the user.active is set to false (see action item 2 in issue).

  2. I don't see application code added here that updates deletedAt when a user is deleted. Is there something I'm missing, or does it need to be implemented?

Thanks!

@srodif
Copy link
Author

srodif commented Sep 10, 2024

Hello @kyleecodes !

You are right, I missed the second action item. I may have been confused with the title and scope of issue #531. So, for your first point I will add the code. For the second point, I also want to make the addition, I am just not sure if this is taking over #531 too.

Thank you for your time and response!

@eleanorreem
Copy link
Contributor

@srodif Thanks for your great work so far!

The migration doesn't cover the work needed for #531. The migration is a data cleanup of previous entries. #531 covers changing the service method going forward. Hope that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants