D23979: Add missing parameters to Permission related jobs and fix url typo.

[Daniel Vrátil]

dvratil requested changes to this revision.
dvratil added a comment.
This revision now requires changes to proceed.


  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).

INLINE COMMENTS

> permissioncreatejob.cpp:84
> +
> +    if (!useDomainAdminAccess) {
> +        query.addQueryItem(QStringLiteral("useDomainAdminAccess"), Utils::bool2Str(useDomainAdminAccess));

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?

> permissiondeletejob.cpp:132
> +    query.addQueryItem(QStringLiteral("supportsAllDrives"), Utils::bool2Str(d->supportsAllDrives));
> +    if (!d->useDomainAdminAccess) {
> +        query.addQueryItem(QStringLiteral("useDomainAdminAccess"), Utils::bool2Str(d->useDomainAdminAccess));

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?

> permissionmodifyjob.cpp:77
>  
> +    if (!removeExpiration) {
> +        query.addQueryItem(QStringLiteral("removeExpiration"), Utils::bool2Str(removeExpiration));

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...

REPOSITORY
  R477 KGAPI Library

REVISION DETAIL
  https://phabricator.kde.org/D23979

To: barchiesi, dvratil
Cc: #libkgapi, kde-pim, barchiesi, fbampaloukas, dvasin, rodsevich, winterz, vkrause, mlaurent, knauss, dvratil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20190916/0f5ed609/attachment.html>