-
Notifications
You must be signed in to change notification settings - Fork 16
feat: Implement quota rules #255
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
base: main
Are you sure you want to change the base?
Conversation
1a727e5
to
0c24821
Compare
5d30be7
to
2d185f1
Compare
13fccfc
to
3170f18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3ba8178
to
e9df9be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The "Select users/group" NcSelect could use a max width
- The save button could appear only if needed
- The "Rules can be set for specific groups..." NcNoteCard should be displayed after the h2 IMO
How can one have a usage AND a shared usage if the rule overrides the global settings? Is it because a shared rule and a non-shared rules can both apply to a specific user? No overriding there? It deserves some explanation in the UI and in the code.
e9df9be
to
ce490bd
Compare
Not sure there is a better naming. A good explanation in the UI would be better already. It could include a small example like: |
ce490bd
to
f62cff0
Compare
Implemented all of these and also disabled the save button while it is being saved and updated the screenshot that shows the new info card. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
If you add migration steps in a PR, always bump the app version number so the people checking out your branch (or later using the main branch) will trigger those migrations.
-
Mappers are fine like they are now. Small remark: Instead if implementing add, update and delete methods in the mappers, you can directly use the base methods of mappers:
- create an entity with
$e = new MyEntityClass();
$e->setColA($valA);
$e->setColB($valB);
$e = $myEntitymapper->insert($e);
- delete with
$myEntitymapper->delete($myEntity);
- update with
// once you got the entity
$myEntity->setColB($newValB);
$myEntitymapper->update($myEntity);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks nice!
some UI suggestions:
-
unrelated to this PR sorry, but can you limit the max width of the settings too? The upper part is from Assistant
-
upper and lower padding correction here so there is a little space above and a little less below:
-
A per-user quota for each quota type can be set. If the user has not provided their own API key, this quota will be enforced.
this should be modified to say that it's only used for users/groups not in any of the below quota rule if I understood it correctly.
-
a. the same upper and lower padding fix can be done here too.
b. the two input fields for the value and days/months should be aligned vertically
c. value validation should be done. "Quota rules" sections does it well but a frontend check might save a request ;)
56bd6d5
to
e9a1575
Compare
I think I got everything now and rebased the branch.
For the quota period if you set the length to something less than 1 the backend already made sure the value is not less than 1 and the day is between 1 and 28 when that is used. |
b8dbddd
to
3459b82
Compare
Signed-off-by: Lukas Schaefer <[email protected]>
Signed-off-by: Lukas Schaefer <[email protected]>
3459b82
to
ca2ae11
Compare
Signed-off-by: Lukas Schaefer <[email protected]>
ca2ae11
to
896f0a7
Compare
sure the backend does but it should not just silently correct it, it should throw an error stating why. Even the frontend should have this check. I think there might be min and max attributes that can be used here too. |
359b5fe
to
057d74c
Compare
Signed-off-by: Lukas Schaefer <[email protected]>
057d74c
to
b46b812
Compare
I have the backend now fail with an invalid value and there are min and max values that also prevent saving on the front end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good otherwise!
const typeIconClass = { | ||
user: 'icon-user', | ||
group: 'icon-group', | ||
const typeIconClass = ['icon-user', 'icon-group'] | ||
const ENTITY_TYPES = { | ||
user: 0, | ||
group: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool!
state: loadState('integration_openai', 'rules', []).map((rule) => { | ||
rule.pool = rule.pool === 1 | ||
return rule | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may be better suited in the backend
<NcAvatar | ||
v-if="option.entity_type === ENTITY_TYPES.user" | ||
:user="option.entity_id" | ||
:show-user-status="false" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah just noticed there were api calls for user status for all the users. In NC/vue 9, this option has changed to hide-status
https://nextcloud-vue-components.netlify.app/#/Components/NcAvatar
Adds quota rules that allow you to set a quota for user/group and combinations of them including pools (where quota is shared among a collection of users/groups)


Resolves: #149
Todo