Pull request for changes to agenda functionality and new functionality for editing users

[HenrikEngen]

Added duration to the agenda system Added new function for editing users

[simsine]

So from what i have been able to gather we currently don't utilize the SQL transactions correctly. I believe the transactions are automatic, but are only rolled back if an exception is thrown. Now i don't know how we would do this while still returning the error in the response the same way.

[simsine]

One idea would be to instead of having an early return in each case, we could instead set the relevant error message, then throw an exception which we catch, then return a response with the error message. This could work if my assumptions for how this transaction system works is correct. But again i think the way our program is structured it is hard to see past the abstractions when we are using dependency injection without any form of type hints or anything for the entity manager objects we are using.

[petterroea]

@simsine Hello,

Transactions are working as expected - transactions are rolled back if the view handler fails. The intention is to stop buggy code from causing incorrect states in the database, and is what Pyramid recommends themselves.

I recommend against throwing exceptions as they are slow and hard to read - you should never use an exception to handle code flow. I have already instructed @HenrikEngen on how to write this PR properly and I am waiting for him to do so.

[petterroea]

A better way of fixing this PR would be to flatten the if's and go for a "happy path" approach where fields are validated one after another.

But @HenrikEngen should remember we have the @validate decorator that can handle a lot of the type validation being done manually here.

[simsine]

@simsine Hello,

Transactions are working as expected - transactions are rolled back if the view handler fails. The intention is to stop buggy code from causing incorrect states in the database, and is what Pyramid recommends themselves.

I recommend against throwing exceptions as they are slow and hard to read - you should never use an exception to handle code flow. I have already instructed @HenrikEngen on how to write this PR properly and I am waiting for him to do so.

Yeah alright i get that exceptions aren't the correct way of doing it here, i was inclined to do it that way because of what i could gather from the documentation and how it would be done in Java JPA which i am working with currently.

The behaviour that Henrik described to me was that if you submit a request with a field that gets invalidated the request returns early with the completed changes being kept and the remaining changes being thrown out, which makes it behave inconsistently. Wouldn't we want the entity manager to only commit if all field checks are passed for the request?

And i still stand by the fact that the api's we use here are kinda hard to interperet by the way we use dependency injection.

[petterroea]

@simsine The correct behavior in this case would be to perform all the validation before the database object is mutated. By using @validate where possible that is ensured.

As I said, transactions are working as intended - if the function crashes, the transaction is rolled back. If the function chooses to mutate a database object and then return early - how is the transaction manager supposed to know that it wasn't supposed to commit that? It is the view handler writers responsibility to write a view handler that only mutates an object when it intends to "commit" to the mutation being made. The only reason this bug exists in the first place is because of bad code.

And i still stand by the fact that the api's we use here are kinda hard to interperet by the way we use dependency injection.

There is no dependency injection. It's all just plain python function calls. The closest you get are the custom fields added to the request object.