6681:should allow for comments in bulk permissions screen

[ralphmensah]

resolves #6681

[ralphmensah]

#6704

[davidwatkins73]

Thanks, this looks awesome! Will give it a spin and merge.

[davidwatkins73]

Hi @ralphmensah. Just tried this out on my dev machine and the capture part is working perfecly, however I noticed it does not record a change_log message in the database.

If we look at the role editor for a single user, we get comments like:

Looking at the code, it looks like we may need to change the UserRoleService. e.g.

Set<Tuple2<String, String>> usersAndRolesToUpdate = mkBulkOperationPreviews(lines)
                .stream()
                .filter(p -> p.status() == BulkUserOperationRowPreview.ResolutionStatus.OK)
                .map(d -> tuple(d.resolvedUser(), d.resolvedRole()))   // <---- the result tuple needs to include the resolvedComment
                .collect(Collectors.toSet());

And then in the switch statment you may need to pull each part out to a function e.g.

    case ADD:ONLY:
        return bulkHandleAddOnly(usersAndRolesToUpdate);

    ....

private int bulkHandleAddOnly(Tuple3<String, String, String> xs) {
    xs.stream()
       .map(t -> /* create changelog entry record */ )
       .collect(Collectors.collectingAndThen(toSet(), dsl::batchInsert)
       .execute();


    return userRoleDao.addRoles(xs);
}

To check the result, ensure a person table entry has the name of a user, you can then look on the person page and open the Changes section.

[ralphmensah]

@davidwatkins73 can you please direct me where to find the change logs on waltz

[davidwatkins73]

The change logs can be see via the UI in the Changes section for a page, e.g. a person page

To see recent change logs in the database you can use a qry similar to :

select * 
from change_log 
where parent_kind = 'PERSON' 
order by created_at desc;

[mcleo-d]

Fantastic collaboration @ralphmensah and @davidwatkins73 😸

[ralphmensah]

Hi @davidwatkins73 i noticed in creating a change log we get a person by using the userId (which turns out to be the email)

    public int updateRoles(String userName, String targetUserName, UpdateRolesCommand command) {
        LOG.info("Updating roles for userName: {}, new roles: {}", targetUserName, command.roles());

        Person person = personService.getPersonByUserId(targetUserName);
        if(person == null) {
            LOG.warn("{} does not exist, cannot create audit log for role updates", targetUserName);
        } else {
            ImmutableChangeLog logEntry = ImmutableChangeLog.builder()
                    .parentReference(mkRef(EntityKind.PERSON, person.id().get()))
                    .severity(Severity.INFORMATION)
                    .userId(userName)
                    .message(format(
                            "Roles for %s updated to %s.  Comment: %s",
                            targetUserName,
                            sort(command.roles()),
                            StringUtilities.ifEmpty(command.comment(), "none")))
                    .childKind(Optional.empty())
                    .operation(Operation.UPDATE)
                    .build();
            changeLogService.write(logEntry);
        }

        return userRoleDao.updateRoles(targetUserName, command.roles());
    }

this particular code breaks at line:
Person person = personService.getPersonByUserId(targetUserName);
since the user is not found in the person table.
this also happens for individual role changes.

PS: this is a user i created using the `add new user+` link in the individual user admin page

[ralphmensah]

I am currently trying to use the same approach in writing the changelog to the changelog table
@davidwatkins73 can you please have a look.

[mcleo-d]

Hey @davidwatkins73 and @JWoodland-Scott 👋🏻

Would you mind taking a look at the following comment raised by @ralphmensah as he's super eager to move forward with the pull request and get it merged into Waltz.

#6704 (comment)

Kind regards,

James.

[jessica-woodland-scott-db]

Hi @ralphmensah,

Thanks for reaching out. We usually expect all users of Waltz to have a record in the person table and make an implicit join between email on person and the user_name on user. However, as you point out, this doesn't always work particularly well when users are created through the UI.

You could fix this short term by adding the person record in for the user; long term we already have an existing issue #6477 which looks to better improve this process and link the two data points together.

As the edit/owner permissions are a combination of the person's involvement to the entity and the roles a user may have (not dependent on the entity), these two will stay connected. Hence the changelog often links back to the person record and when displaying in the 'Changes' section we try to look up the person record for the change.

[davidwatkins73]

Sorry for the delay in getting back to you.

@ralphmensah are you happy with what needs to be done on this?
More than happy to jump on a call/screenshare if you'd like to go through in more detail.

[ralphmensah]

Hi @ralphmensah,

Thanks for reaching out. We usually expect all users of Waltz to have a record in the person table and make an implicit join between email on person and the user_name on user. However, as you point out, this doesn't always work particularly well when users are created through the UI.

You could fix this short term by adding the person record in for the user; long term we already have an existing issue #6477 which looks to better improve this process and link the two data points together.

As the edit/owner permissions are a combination of the person's involvement to the entity and the roles a user may have (not dependent on the entity), these two will stay connected. Hence the changelog often links back to the person record and when displaying in the 'Changes' section we try to look up the person record for the change.

thanks for the reply. This provided in depth knowledge on what exactly needed to be done, Kindly review my latest commit

[ralphmensah]

Sorry for the delay in getting back to you.

@ralphmensah are you happy with what needs to be done on this? More than happy to jump on a call/screenshare if you'd like to go through in more detail.

Thanks for the reply, kindly review my latest commit

[davidwatkins73]

Changes look great. We will merge this and demo to the team that requested it. Will let you know if they have any feedback.