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

Unexpected hours_til_expiration behavior #498

Open
zpchavez opened this issue May 7, 2021 · 3 comments
Open

Unexpected hours_til_expiration behavior #498

zpchavez opened this issue May 7, 2021 · 3 comments
Labels
blocked by dev Automatically assigned when marking as blocked, needing developer involvement. bug Something isn't working minor Severity: Affects minor functionality or non-critical data. It has an easy workaround.

Comments

@zpchavez
Copy link
Contributor

zpchavez commented May 7, 2021

  1. There is an hours_til_expiration column in the client table that defaults to 24
  2. The /api/invite and /api/resend-invite/{userId} endpoints accept an hours_til_expiration param
  3. If not provided in the request payload, these values default to 48 instead of the client-level default.
  4. The client-level default is used in verification emails and forgot password emails, just not invites.

It seems odd that the client-level default is overridden for invites. However, removing the route-level default of 48 would be a breaking change and would reduce the invite expiration time for existing projects that don't explicitly pass an hours_til_expiration in their invites. Maybe we should add a hours_til_invite_expiration column that defaults to 48 and remove the default defined at the route level. Then existing functionality would be preserved.

The reason this came up for me is because we wanted to extend invite expiration time and I tried to do it by updating hours_til_expiration in the client table.

@zpchavez zpchavez added the bug Something isn't working label May 7, 2021
@zpchavez
Copy link
Contributor Author

zpchavez commented May 7, 2021

There should probably be some discussion about this before it is worked on

@zpchavez
Copy link
Contributor Author

zpchavez commented May 7, 2021

I also just noticed that the endpoint param spells till with two Ls. Maybe we can fix that but also continue accepting two Ls for backwards compatibility? And then in 3.0 we can introduce a breaking change and leave it to one L always.

@synbot synbot added the blocked by dev Automatically assigned when marking as blocked, needing developer involvement. label May 13, 2021
@bobeagan bobeagan added the minor Severity: Affects minor functionality or non-critical data. It has an easy workaround. label May 13, 2021
@spruce-bruce
Copy link
Collaborator

hours_til_expiration is a but of a bummer field name now in retrospect. I'd like to go hunt down when it was added and try and remember the whole context to understand if I want 2 columns or 1.

At the very least some kind of column rename and possibly abstraction is probably in order here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked by dev Automatically assigned when marking as blocked, needing developer involvement. bug Something isn't working minor Severity: Affects minor functionality or non-critical data. It has an easy workaround.
Projects
None yet
Development

No branches or pull requests

4 participants