This patch adds missing parameters to Permission related Jobs, necessary for sending invitation emails and operating ad domain admin.
Details
Details
Diff Detail
Diff Detail
- Repository
- R477 KGAPI Library
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Nice, thanks for the patch.
I understand the desire to not sent unnecessary query parameters when they do not differ from the default value, but the way the code stands now it look like "magic" to me and is error prone - how about defining default value constants like e.g.
namespace {
static constexpr bool useDomainAdminAccessDefault = false;
}and then all the conditions would look like
if (d->useDomainAdminAccess != useDomainAdminAccessDefault) {
...
}Which I think is not only much easier to understand but also prevents mistakes like those below and makes changing the default super-easy (single place in the entire file).
| src/drive/permissioncreatejob.cpp | ||
|---|---|---|
| 101 | This property defaults to false, so shouldn't the condition be the other way around, so it gets sent when it's different from the default? | |
| src/drive/permissiondeletejob.cpp | ||
| 136 | This property defaults to false, so shouldn't the condition be the other way around, so it gets sent when it's different from the default? | |
| src/drive/permissionmodifyjob.cpp | ||
| 95 | This property defaults to false, so shouldn't the condition be the other way around, so it gets sent when it's different from the default? Same below... | |
Comment Actions
That sounds like a better way of handling defaults, I'll keep it in mind when touching other Jobs.
Comment Actions
Thanks for the fix. Could you also modify the constructors so that the class members are initialized using the fooDefault constants as well, so that we really only have a single place in the entire file where the default value is defined?
Comment Actions
@dvratil I rechecked if Permission was complete and added missing attributes. Also added an example.