Enhance blocking workers flow

[peregrineshahin]

• Old messages become immutable
• The message is required thus explaining more the reason why would an approver/the same user unblock/block.
• Full history of the blocking notes
• automatic inclusion for timestamp and blocker name

[vdbergh]

Looks like a big improvement...

[vdbergh]

The message field in the worker collection currently has a size limit of 1024 characters (enforced by the schema). This was to avoid that the user could "accidentally" paste a large amount of data in the field. With the current PR this size restriction in the back end should be moved to the front end,

Personally I think it would be more natural to replace the message field by an array of message fields but this would require a db conversion.

[peregrineshahin]

Why would it need a db conversion? The old data about old notes for blocked workers is not of significance as of now..

[vdbergh]

Of course it does not strictly need a db conversion since the combined schema (a single message or an array of messages) can be handled appropriately in the front end. But this is more messy. But I guess during migration it would be necessary anyway.

[vdbergh]

Unless you are suggesting simply deleting the old records in the db. Which I guess is also a kind of conversion :)

[peregrineshahin]

Unless you are suggesting simply deleting the old records in the db. Which I guess is also a kind of conversion :)

Indeed I pushed your suggestion, it looks like there would still be two todos that could be hacked with the .get() method right now, and the old field yes would need to be removed.
Please review the new commits.

[vdbergh]

If you want to enforce that either "message" or "notes" is present then you can do it as follows.

worker_schema = intersect(
  {
      "_id?": ObjectId,
      "worker_name": short_worker_name,
      "blocked": bool,
      "message?": worker_message,  # old field, todo: remove this field from db if exists
      "notes?": intersect(
          [
              {"time": datetime_utc, "username": username, "message": worker_message},
              ...,
          ],
          size(0, 100),  # new field, todo: add this field to db if it doesn't exists
      ),
      "last_updated": datetime_utc,
  },
  one_of("message", "notes"),
)

EDIT. See below for a more correct version.

[vdbergh]

I guess one wants "message", "notes" or both to be present. So it should be

worker_schema = intersect(
  {
      "_id?": ObjectId,
      "worker_name": short_worker_name,
      "blocked": bool,
      "message?": worker_message,  # old field, todo: remove this field from db if exists
      "notes?": intersect(
          [
              {"time": datetime_utc, "username": username, "message": worker_message},
              ...,
          ],
          size(0, 100),  # new field, todo: add this field to db if it doesn't exists
      ),
      "last_updated": datetime_utc,
  },
  at_least_one_of("message", "notes"),
)

EDIT. But since this is supposed to be transitional, perhaps it is not worth trying to be extremely precise in the schema...

[peregrineshahin]

EDIT. But since this is supposed to be transitional, perhaps it is not worth trying to be extremely precise in the schema...

Indeed but I'm inclined to have everything in place for correctness, so that one understands what's up when doing the todo's, so added that change, thanks for the review.

[peregrineshahin]

one thing I generally don't like is that we do replace_one with a whole new list each time instead of update_one and using push https://www.mongodb.com/docs/manual/reference/operator/update/push/
I actually wanted to push a new message into the array without fetching it, that'd be generally more efffecient since the more work one leaves to the database the better scalable it always becomes and I expect working on large data might show the difference. but the problem I faced is that the verification using the schema becomes troublesome.

[vdbergh]

I agree with what you say but I don't know a solution with the standard mongodb approach. I.e. cramming as much data as possible in a single record (or "document" in mongodb parlance).

One solution would be to take a relational approach instead. I.e. two collections, one for workers and one for notes (where a record in the notes collection has a field with the id of the corresponding worker record).