[24.1] Allow to change only the description of a quota

[bernt-matthias]

A quota update via API requires name or quota to be set, but the manager requires a name to be set.

If one wants to change just the description of a quota the used select statement gets the quota itself and the elif branch hits resulting in an exception.

With the change one can change just the description, but one still needs to specify the name of the quota. We could hange this such that also in the manager the name of the quota needs to be specified.

I guess in the admin UI this has not been a problem because the name and description are prefilled with the old values.

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[jdavcs]

Since we can now update just the description, this message needs to be edited too. I'd keep it simple (e.g. "has been updated") and not worry about specifying what exactly was updated (name or description or both, etc.)

[jdavcs]

@bernt-matthias Sorry, I'm confused; can you, please, explain why "If one wants to change just the description of a quota the used select statement gets the quota itself and the elif branch hits resulting in an exception."? I'm looking at this:
elif params.name != quota.name and self.sa_session.scalars(stmt).first():.

[mvdbeek]

Those messages are not the most insightful anyway, I would follow up on dev and remove them all. Either we got a 200 response and it's good or we got an exception.

[jdavcs]

Those messages are not the most insightful anyway, I would follow up on dev and remove them all. Either we got a 200 response and it's good or we got an exception.

Strongly agree. An update can mean a lot of things, including any combination of updated values, some derived from others, etc. Besides, even if we constructed a detailed message, I don't think it would be useful. And a generic "has been updated" message, if at all needed, can be constructed on the client.

[bernt-matthias]

@bernt-matthias Sorry, I'm confused; can you, please, explain why "If one wants to change just the description of a quota the used select statement gets the quota itself and the elif branch hits resulting in an exception."? I'm looking at this: elif params.name != quota.name and self.sa_session.scalars(stmt).first():.

Lets say there is a quota with name "A" and description "", and I want to update the description to "Description of A". Then the function will get params.name = "A" and the new description. Then stmt = select(Quota).where(Quota.name == params.name) will return the quota which I try to update (actually it is the same as the argument quota). I.e. the old code will raise ActionInputError("A quota with that name already exists."). Which is kind of True but not what should actually be checked here:

Instead of checking: Is there a quota with name X it needs to check is there another quota with name X.

[jdavcs]

Lets say there is a quota with name "A" and description "", and I want to update the description to "Description of A". Then the function will get params.name = "A" and the new description. Then stmt = select(Quota).where(Quota.name == params.name) will return the quota which I try to update (actually it is the same as the argument quota). I.e. the old code will raise ActionInputError("A quota with that name already exists."). Which is kind of True but not what should actually be checked here:

Instead of checking: Is there a quota with name X it needs to check is there another quota with name X.

Thanks! But still, I'm looking at the old code: it'll raise an error only if both conditions are true:
params.name != quota.name and self.sa_session.scalars(stmt).first() - i.e., "I am changing the name of quota" AND "a quota with this new name exists". So, if I'm NOT changing quota.name, then the first expression will evaluate to False and the exception won't be raised, right? Am I misreading this logic?

That said, I think your version is better, because it's immediately clear from looking at the statement that it answers the question "is there a quota record with that name that is not the quota I am editing?" - which is very easy to read!

Also, with this statement, I think you don't need to check params.name != quota.name in the elif clause anymore.

[bernt-matthias]

Thanks! But still, I'm looking at the old code: it'll raise an error only if both conditions are true: params.name != quota.name and self.sa_session.scalars(stmt).first() - i.e., "I am changing the name of quota" AND "a quota with this new name exists". So, if I'm NOT changing quota.name, then the first expression will evaluate to False and the exception won't be raised, right? Am I misreading this logic?

That is correct.

That said, I think your version is better, because it's immediately clear from looking at the statement that it answers the question "is there a quota record with that name that is not the quota I am editing?" - which is very easy to read!

:)

Also, with this statement, I think you don't need to check params.name != quota.name in the elif clause anymore.

I think we should not change this in a bug fix.

But I think you are right as long as we assume that we have unique names. I was wondering if the attempt to introduce a change to the DB that would violate unique names would raise an exception anyway, i.e. are our if statements redundant anyway?

[jdavcs]

Also, with this statement, I think you don't need to check params.name != quota.name in the elif clause anymore.

I think we should not change this in a bug fix.

I think it's part of the bug fix: we've effectively moved that check into the statement filtering clause, so we don't need to check it again.

But I think you are right as long as we assume that we have unique names. I was wondering if the attempt to introduce a change to the DB that would violate unique names would raise an exception anyway, i.e. are our if statements redundant anyway?

We can assume that: there's a unique constraint on that field. We could delegate that to the database; however, that's a little more involved. You'd wrap the session.execute in a try/except block, and you'd need to account for different databases to be certain that it's the unique constraint that's been violated, and not some other database error. It could be done like this with this determining that the constraint was violated. But I think for what we need here, the current approach is simpler and is sufficient.

[jdavcs]

@bernt-matthias Everything looks good, thanks! Would you mind rebasing to get rid of the last empty commit (for a cleaner history)?

[github-actions]

This PR was merged without a "kind/" label, please correct.

[nsoranzo]

It looks to me that this ended up being more a refactoring than a bugfix? Should the PR title be updated, as it ends up in the release notes?

[bernt-matthias]

It's a mix, but in my opinion more a bugfix, since we changed behavior.