Member: implement timeouts

[Bilal2453]

This will add 3 methods into Member class, :timeoutFor(duration)/:timeoutUntil(Date)/:removeTimeout(). And a single boolean getter .timedout which uses the communication_disabled_until property as its state; this might make the getter a bit expensive to call since it has to create two Date objects and compare them, but it seems like this is the way Discord intend you to do it:

when the user's timeout will expire and the user will be able to communicate in the guild again, null or a time in the past if the user is not timed out

I've tested the changes a bit, everything seem to work as expected, the cache properly gets updated for the state tracking. The doc-comments might need a bit of language review though to make sure it is clear enough.

[SinisterRectus]

Looks great. The Date and Time classes really do most of the heavy lifting here.

Is it worth changing Member:_timeout to a public Member:setCommunicationDisabledUntil?

[Bilal2453]

The Date and Time classes really do most of the heavy lifting here.

Yep, they came in handy. Probably a good reason to keep/implement them in Discordia 3 as well.

Is it worth changing Member:_timeout to a public Member:setCommunicationDisabledUntil?

I cannot think of any scenario in which this can be useful for the end user. If they got an ISO string they can easily parse that into a Date object (does mean the performance is a bit affected since it will be converted into an ISO string again but that should be fine), without mentioning there is no good scenario in which the user input has to be ISO string.

Also because of that, I intentionally let any value acceptable, so if they want to do :timeoutUntil("2022-01-15T16:48:51+00:00") or :timeoutFor(false) (to remove the timeout), or any other raw value Discord accept, it should work. So I don't really see that being much of a useful method, therefore probably not worth it.

[SinisterRectus]

What happens if you try to set a timeout for a member that is already timed out?

[Bilal2453]

What happens if you try to set a timeout for a member that is already times out?

The timeout gets updated to the new one you set, for example, if a member have a 60 second timeout, and you call this again with 120 second, Discord will set a new timeout that's 120 second exactly.

If the user does not have a timeout, and you try to unset any timeout (that is, passing null) the request success without Discord doing anything.

[RiskoZoSlovenska]

I can see someone wanting to get the date someone is timed-out until; perhaps it would be better for timedout to return a Date if the member is timed out, and nil if they aren't. And in that case, it might be better to rename it to timedoutUntil or something similar.

[Bilal2453]

That's a good point, I haven't thought about that. I don't know if it is a good idea for timedout to return anything other than a boolean, it kind of does not make sense. Perhaps we should implement a second getter? .communicationDisabledUntil

[RiskoZoSlovenska]

Why not just rename it to timedoutUntil? Users can still use the return value in an if statement, since a Date is truthy.

[Bilal2453]

We are storing the state as an ISO string, we cannot store it as a Date object, because if for example the member is not cached, and a timeout was added (either by another bot or using the Discord client) the member cache will be updated with communication_disabled_until set to an ISO timestamp. Therefor each call to timedoutUntil will basically be doing the following:

1. Take the input and convert it into a Date object. (When you do use timeoutFor/timeoutUntil)
2. Convert the date object into an ISO timestamp.
3. Convert the timestamp back into a Date object. That sounds.. rather ridiculous to me; So another property may indeed make more sense.

[RiskoZoSlovenska]

I don't really understand what you mean. Why not just do something like

function get.timedoutUntil(self)
    local state = self._communication_disabled_until
    if state then
        local untilDate = Date.fromISO(state)

        if untilDate > Date() then
            return untilDate
        end
    end
    return nil
end

which is not far at all from the current implementation.

[SinisterRectus]

Just keep it as a string, like the joinedAt property.

[Bilal2453]

I don't really understand what you mean. Why not just do something like

function get.timedoutUntil(self)
  local state = self._communication_disabled_until
  if state then
      local untilDate = Date.fromISO(state)

      if untilDate > Date() then
          return untilDate
      end
  end
  return nil
end

which is not far at all from the current implementation.

This is exactly what I meant. This is the third conversion you have to be making here. The first one is when a user uses timeoutFor, the input is converted into a Date object, parsed into ISO and sent. This takes the timestamp, parse it, and make a Date object for it.

Here:

function Member:timeoutFor(duration)
    if type(duration) == 'number' then
        duration = (Date() + Time.fromSeconds(duration)):toISO()
    elseif isInstance(duration, Time) then
        duration = (Date() + duration):toISO()
    end
    return self:_timeout(duration)
end

This creates the Date object and then parses it into iso timestamp to be sent (Discord only accept that). Then you use timedoutUntil, which reads the _communication_disabled_until state (that was already set before if the user had used timeoutFor) and then -again- converts it back into a Date object.

Basically what I am saying is that you are duplicating the steps.

Just keep it as a string, like the joinedAt property.

Sounds like a good call, although should this be its own property is the question

[SinisterRectus]

Oh, I didn't realize it wasn't...

Does the API provide the string?

[Bilal2453]

Oh no, Discord itself do provide the timestamp string of course. It is just that if the user had already used the method to set the timeout then tries to read timedoutUntil (which would return a Date object) we would be repeating ourselves.

[Bilal2453]

So I guess we shall make timedout return the iso timestamp if the member was timedout (the timedout can be an iso timestamp in the past if a timeout was set and then ended), if they were not we return a nil?

[RiskoZoSlovenska]

Oh, you're assuming the user will be calling timedoutUntil right after timing a member out. In that case, why not make :_timeout() take a Date object, convert it to ISO and then return the Date object back?

[RiskoZoSlovenska]

So I guess we shall make timedout return the iso timestamp if the member was timedout (the timedout can be an iso timestamp in the past if a timeout was set and then ended), if they were not we return a nil?

For consistency's sake (with joinedAt, as Sinister mentioned), yeah, I think returning just the ISO string would be reasonable. In 3.0 though, IMO it would be nice if both properties returned a Date.

[Bilal2453]

I actually just realized we are exactly doing that right now huh, since Discord is sending a string timestamp, and the timestamp may be in the past if it was set then expired, I was already creating the Date object in timedout either ways. Right Risko, kind of forgot about that sorry.

function get.timedout(self)
    local state = self._communication_disabled_until
    if not state then
        return false
    else
        return Date.fromISO(state) > Date()
    end
end

The real issue would be just consistency

[SinisterRectus]

Just return _communication_disabled_until as it is. Call it timedOutUntil. Uppercase O because timed out is two words.

[Bilal2453]

Do we remove the current timedout? Just always returning the raw _communication_disabled_until is a bit of trouble for the user since they would have to compare it against the current date to know if the member is currently timed out or not

[SinisterRectus]

It's convenient, but adds complexity. I can go either way. If you keep it, change it to timedOut.

[Bilal2453]

This should be sufficient now. Do review the doc comment for timedOutUntil though

[Bilal2453]

I guess merging with squash is a good idea now.

(See on the right of the merge button, an arrow that shows a drop menu)

[SinisterRectus]

I've always squashed recently.