Closes #2540 & #2614

[lbownik]

No description provided.

[diasf]

Added but not used.

[lbownik]

fixed

[diasf]

Shouldn't this new column also be considered in equals/toString below?

[lbownik]

fixed

[diasf]

You added DatasetPage.isViewedFromAnonymizedPrivateUrl but didn't use it.

[lbownik]

i did in metadataTab.xhtml
<ui:param name="isViewedFromAnonymizedPrivateUrl" value="#{DatasetPage.isViewedFromAnonymizedPrivateUrl()}"/>

[diasf]

Yeah I know, just thought it could also be used here and in the method below as well. But not insisting.

[lbownik]

fixed

[diasf]

The isAffiliatedWith name is a bit misleading. In dataverse affiliation is usually used between users and organizations. Not sure actually if it's worth having this method for all users, it seems to make sense only for PrivateUrlUser, so maybe we could just have it there (same for isAnonymized)?

[lbownik]

Having these methods in User class hierarchy simplifies client code and is more elegant than using instanceof operator (my opintion, you may disagree)
isAffiliatedWith - so maybe "isRelatedWith" or isAllowedToView ? (any proposals) ?

[diasf]

Having these methods in User class hierarchy simplifies client code and is more elegant than using instanceof operator (my opintion, you may disagree)

Would agree that instanceof is usually a sign of sub-optimal design. But adding methods to an interface which will only be used for a specific implementation does also violate the interface segregation principle. And it looks like that's the case here as the only place we use those two new methods are for instances of PrivateUrlUser:

    public boolean isViewedFromPrivateUrl(final DvObject dataset) {
        return getUser() instanceof PrivateUrlUser && getUser().isAffiliatedWith(dataset);
    }
    
    public boolean isViewedFromAnonymizedPrivateUrl(final DvObject dataset) {
        return getUser().isAnonymized() && isViewedFromPrivateUrl(dataset);
    }

By that I mean, the above methods specifically have ...PrivateUrl(.. in their name, so we're not concerned about any other types of users.

isAffiliatedWith - so maybe "isRelatedWith" or isAllowedToView ? (any proposals) ?

If we're accepting that we're in the context of PrivateUrlUser, then I would say you don't need that method at all and could just do:

    public boolean isViewedFromPrivateUrl(final DvObject dataset) {
        return getPrivateUrlUser().exists(u -> u.getDatasetId() == dataset.getId());
    }
    
    public boolean isViewedFromAnonymizedPrivateUrl(final DvObject dataset) {
        return getPrivateUrlUser().exists(PrivateUrlUser::isAnonymized) && isViewedFromPrivateUrl(dataset);
    }

    private Option<PrivateUrlUser> getPrivateUrlUser() {
        return Option.of(getUser()).filter(PrivateUrlUser.class::isInstance).map(PrivateUrlUser.class::cast);
    }

Just a proposition. If you want to keep the method in PrivateUrlUser, then isAllowedToView would be fine, I think.

[lbownik]

yes I'm aware that I have conflicting situation here - striving for encapsulation and genericness naturally violates interface segregation principle (eg. Object.hashCode violates it) - it is a contentious issue

I renamed isAffiliatedWith into isAllowedToView and changed Guest.isAllowedToView to return true as this makes more sense (though Guest.isAllowedToView implementation is not used in any usecase)

[diasf]

yes I'm aware that I have conflicting situation here - striving for encapsulation and genericness naturally violates interface segregation principle (eg. Object.hashCode violates it) - it is a contentious issue

So let's change it, and remove those methods from User. Genericness is not giving us anything here, as we don't even make reasonable use of it right from the start.

I agreed on isAllowedToView name only under the condition that it's for PrivateUrlUser only. For other implementations of User the method (at least with the current implementation) doesn't make much sense. It's not like someone who calls user.isAllowedToView(dataset) would get a correct response which would account for access permissions.