Skip to content
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

S24 privacy combined profile #1060

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from
Open

Conversation

jsenning
Copy link
Contributor

@jsenning jsenning commented Jul 9, 2024

Generalize privacy settings for items appearing in a user's profile.

This is a large PR and reflects work done during two consecutive summer practicums. It supersedes PR #1042 and #937 and is partnered with PR gordon-cs/gordon-360-ui#2355 in gordon-360-ui.

See gordon-cs/gordon-360-ui#2355 for a description.

@jsenning jsenning marked this pull request as ready for review July 10, 2024 21:09
@jsenning jsenning requested review from russtuck and EjPlatzer July 10, 2024 21:09
@Luke-W-Hart Luke-W-Hart added the s24 Summer Practicum 2024 label Jul 11, 2024
Copy link
Member

@russtuck russtuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks very good, but I have a few suggestions and questions.

@@ -635,7 +635,11 @@ private async Task<bool> CanUpdateAsync(string resource)

return false;
}

case Resource.PROFILE_PRIVACY:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used? Should it be?
(Is it only supported for facstaff?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new privacy settings now are available to all users, but the call to StateYourBusiness() in the ProfileController is commented out and that is the only place this resource is used. One way to address this would be to uncomment the StateYourBusiness() reference but have it just return true for all users rather than returning true only for facstaff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the profile controller does call StateYourBusiness() with the PROFILE_PRIVACY resource. SYB() always returns true, so behavior is not changed but this makes it easier to add some restrictions if needed in the future.

Gordon360/Static Classes/Names.cs Outdated Show resolved Hide resolved

namespace Gordon360.Models.ViewModels
{
// Field: the field where the user wants to update the privacy setting (HomeCity, HomeState, HomeCountry, SpouseName, Country, HomePhone, MobilePhone)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Field: the field where the user wants to update the privacy setting (HomeCity, HomeState, HomeCountry, SpouseName, Country, HomePhone, MobilePhone)
// Field: a list of fields where the user wants to update the privacy setting (HomeCity, HomeState, HomeCountry, SpouseName, Country, HomePhone, MobilePhone)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this suggestion correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think field is a single instance of one of the values listed in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second look, at least one place a comment like this occurs it does seem that Field is a list -- I'll fix the comment.

// Field: the field where the user selected the privacy setting
// (HomeCity, HomeState, HomeCountry, SpouseName, Country, HomePhone, MobilePhone)
// VisibilityGroup: the group that the user wanted to be seen by (Public, Private, FacStaff)
public class UserPrivacyViewModel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused by the difference between 3 very similar UserPrivacy*ViewModel s. Maybe it's just me, but could we make the comments a little clearer about the role and purpose of each one?

Gordon360/Services/ProfileService.cs Outdated Show resolved Hide resolved
Gordon360/Services/ProfileService.cs Show resolved Hide resolved
Gordon360/Services/ProfileService.cs Show resolved Hide resolved
Gordon360/Services/ProfileService.cs Outdated Show resolved Hide resolved

namespace Gordon360.Models.ViewModels
{
// Field: the field where the user wants to update the privacy setting (HomeCity, HomeState, HomeCountry, SpouseName, Country, HomePhone, MobilePhone)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add an overall comment about the purpose of this class. So far, I see that it's used as an argument to UpdateUserPrivacyAsync in ProfileService.cs.

Gordon360/Services/ProfileService.cs Show resolved Hide resolved
@jsenning jsenning requested a review from russtuck July 12, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s24 Summer Practicum 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants