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

Feat: integrate domain delete + renew with DB + tooltips #276

Merged
merged 8 commits into from
Feb 26, 2023

Conversation

Myrfion
Copy link
Contributor

@Myrfion Myrfion commented Feb 24, 2023

  • Add integration of deleting domain
  • Add integration of renewing domain (expiry date + 6 months)
  • Add tooltips for action buttons
  • Add loading on table row action submission

Here is what the table row in the loading state looks like:
Screenshot 2023-02-24 at 11 13 53

Closes #271 #272 #205

@Myrfion Myrfion added this to the Milestone 0.4 milestone Feb 24, 2023
@Myrfion Myrfion self-assigned this Feb 24, 2023
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

This is going to require #274, since these operations require background async jobs to complete. I wonder if we should wait to get that in, so you can call the top-level API bits of those flows?

cc @Genne23v.

const newExpiresAt = new Date(record.expiresAt);
newExpiresAt.setMonth(newExpiresAt.getMonth() + 6);

await updateRecordById(
Copy link

Choose a reason for hiding this comment

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

This signature is extremely hard to use and maintain. Please pass in an object instead. You can check the certificate and record models for an example if you would like to

);
}
} else if (request.method === 'DELETE') {
await deleteRecordById(formalizedID);
Copy link

Choose a reason for hiding this comment

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

If I understand this correctly, this is now what we should do.

If you take the record and delete it from the FE, that will not actually delete the record from route53, it will only delete our knowledge of it.

Instead, we should kick off a worker, that removes the record from route53, then delete it from the db as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like we don't have an api for making a proper deletion yet, I guess it makes sense to wait till some related PRs gonna be merged as like #274 as @humphd mentioned

Copy link

Choose a reason for hiding this comment

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

To be honest, I'd just add a TODO into the code instead of this DB tie and ship this PR.
If a PR is not merged, it's constant work to resolve conflicts and keep it up to date and ready to merge while others are working on the same piece of code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dadolhay makes sense, I did what you say, just left todos comments and opened issues to resolve them when the appropriate code for all these things gonna be shipped #280 #281

@Myrfion Myrfion requested a review from a user February 26, 2023 04:04
@Myrfion Myrfion merged commit f907d4b into main Feb 26, 2023
@Myrfion Myrfion deleted the feature/271/dns-record-deletion-integration branch February 26, 2023 21:47
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.

Record Deletion in Records Table
3 participants