-
Notifications
You must be signed in to change notification settings - Fork 129
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
Enhance blocking workers flow #2113
Enhance blocking workers flow #2113
Conversation
peregrineshahin
commented
Jul 7, 2024
- 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
8c4c089
to
8476cc1
Compare
* 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
8476cc1
to
c1233cb
Compare
Looks like a big improvement... |
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. |
Why would it need a db conversion? The old data about old notes for blocked workers is not of significance as of now.. |
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. |
Unless you are suggesting simply deleting the old records in the db. Which I guess is also a kind of conversion :) |
e61b79f
to
ea8d9bf
Compare
Indeed I pushed your suggestion, it looks like there would still be two todos that could be hacked with the |
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. |
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... |
a76568a
to
da13efb
Compare
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. |
da13efb
to
1c04461
Compare
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 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). |