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

Merge Ratings into AtlText #595

Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
0d83dd4
Update OpenAPI spec
andrewdmontgomery Dec 4, 2024
83f1de6
Add setRating function
andrewdmontgomery Dec 4, 2024
242ccf1
Update OpenAPI spec: Avatar.Rating --> AvatarRating
andrewdmontgomery Dec 5, 2024
b948f51
Add Rating functionality
andrewdmontgomery Dec 5, 2024
c287476
Revert "Update OpenAPI spec: Avatar.Rating --> AvatarRating"
andrewdmontgomery Dec 6, 2024
eade506
Bridge `Avatar.Rating` --> `AvatarRating`
andrewdmontgomery Dec 6, 2024
b2dfa18
Change icon for Rating menu item
andrewdmontgomery Dec 6, 2024
32698cc
Refactor into helper functions
andrewdmontgomery Dec 6, 2024
9cd0a37
Handle errors
andrewdmontgomery Dec 6, 2024
338c965
Add unit tests
andrewdmontgomery Dec 6, 2024
ed91a63
Fix typo in localized string comment
andrewdmontgomery Dec 10, 2024
8f690dd
Use animation when updating the grid after setting the avatar rating
andrewdmontgomery Dec 10, 2024
4d8222d
Add toast message when avatar rating update succeeds
andrewdmontgomery Dec 10, 2024
18701f3
Fix typo in `rating` query item
andrewdmontgomery Dec 10, 2024
5cd6c41
Add unit tests AvatarPickerViewModel
andrewdmontgomery Dec 10, 2024
2eb1a9b
Remove unused queryItem
andrewdmontgomery Dec 10, 2024
a840f86
Refactor data for URLSessionAvatarPickerMock
andrewdmontgomery Dec 10, 2024
89751be
Fix typo in localized string source
andrewdmontgomery Dec 11, 2024
72329da
Update strings in base locale
andrewdmontgomery Dec 11, 2024
b535bac
Merge branch 'trunk' into andrewdmontgomery/qe-avatar-rating
andrewdmontgomery Dec 11, 2024
2c54ae0
QE: Avatar Rating (#589)
andrewdmontgomery Dec 11, 2024
9dc960e
Merge branch 'trunk' into andrewdmontgomery/etoledom/alt-text-editor
andrewdmontgomery Dec 11, 2024
64531f4
Fix merge conflict resolution mistakes
andrewdmontgomery Dec 11, 2024
8547560
Refactor `setRating(_ rating: avatar)` --> `update(_ avatar, rating)`
andrewdmontgomery Dec 11, 2024
7f1fc02
Mark `update(:)` functions as `@discardableResult`
andrewdmontgomery Dec 11, 2024
ff3f1a2
Move ProfileService.setRating() --> AvatarService.update()
andrewdmontgomery Dec 11, 2024
71abf01
Mark the update() function @discardableResult
andrewdmontgomery Dec 11, 2024
c115df7
Removed unused function from ProfileService
andrewdmontgomery Dec 11, 2024
175f490
Move toastMessage out of `withAnimation {}` block
andrewdmontgomery Dec 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions Sources/Gravatar/Network/Services/AvatarService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,22 @@ public struct AvatarService: Sendable {
}
}

package func update(_ altText: String?, rating: AvatarRating?, avatarID: String, accessToken: String) async throws {
@discardableResult
package func update(
_ altText: String?,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
_ altText: String?,
altText: String?,

Now that we have multiple parameters that we can "update", I think the calling site might read better if we name the first parameter. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go with this change 👍

rating: AvatarRating?,
avatarID: String,
accessToken: String
) async throws -> Avatar {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't currently use this return data. But since the API defines a return type for this, I thought we probably should return it, and make it @discardableRersult.

That does mean that if we can't decode it (which I would expect to be quite rare since it would be a backend error that breaks the API), this will throw an error.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually, if (when?) we make this part of the API public, we can follow the pattern of:

public update(::::) --> AvatarType
package update(::::) --> Avatar

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! 👍

var request = URLRequest(url: .avatarsURL.appendingPathComponent(avatarID))
request.httpMethod = "PATCH"
let updateBody = UpdateAvatarRequest(rating: rating, altText: altText)
request.httpBody = try JSONEncoder().encode(updateBody)

let authorizedRequest = request.settingAuthorizationHeaderField(with: accessToken)
do {
_ = try await client.data(with: authorizedRequest)
let (data, _) = try await client.data(with: authorizedRequest)
return try data.decode()
} catch {
throw error.apiError()
}
Expand Down
13 changes: 13 additions & 0 deletions Sources/GravatarUI/Avatar+AvatarRating.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
extension Avatar {
/// Transforms `Avatar.Rating` into `AvatarRating`
/// This is only necessary while we maintain both enums. For our next major realease, `Avatar` will use the `AvatarRating` enum
/// rather than defining its own.
var avatarRating: AvatarRating {
switch self.rating {
case .g: .g
case .pg: .pg
case .r: .r
case .x: .x
}
}
}
21 changes: 21 additions & 0 deletions Sources/GravatarUI/Resources/en.lproj/Localizable.strings
Original file line number Diff line number Diff line change
@@ -1,6 +1,21 @@
/* Rating that indicates that the avatar is suitable for everyone */
"Avatar.Rating.G.subtitle" = "General";

/* Rating that indicates that the avatar may not be suitable for children */
"Avatar.Rating.PG.subtitle" = "Parental Guidance";

/* Rating that indicates that the avatar may not be suitable for children */
"Avatar.Rating.R.subtitle" = "Restricted";

/* Rating that indicates that the avatar is obviously and extremely unsuitable for children */
"Avatar.Rating.X.subtitle" = "Extreme";

/* An option in the avatar menu that deletes the avatar */
"AvatarPicker.AvatarAction.delete" = "Delete";

/* An option in the avatar menu that shows the current rating, and allows the user to change that rating. The rating is used to indicate the appropriateness of an avatar for different audiences, and follows the US system of Motion Picture ratings: G, PG, R, and X. */
"AvatarPicker.AvatarAction.rate" = "Rating: %@";

/* An option in the avatar menu that shares the avatar */
"AvatarPicker.AvatarAction.share" = "Share...";

Expand Down Expand Up @@ -82,6 +97,12 @@
/* This error message shows when the user attempts to delete an avatar and fails. */
"AvatarPickerViewModel.Delete.Error" = "Oops, there was an error deleting the image.";

/* This error message shows when the user attempts to change the rating of an avatar and fails. */
"AvatarPickerViewModel.Rating.Error" = "Oops, something didn't quite work out while trying to rate your avatar.";

/* This confirmation message shows when the user picks a different avatar rating and the change was applied successfully. */
"AvatarPickerViewModel.RatingUpdate.Success" = "Avatar rating was changed successfully.";

/* This error message shows when the user attempts to share an avatar and fails. */
"AvatarPickerViewModel.Share.Fail" = "Oops, something didn't quite work out while trying to share your avatar.";

Expand Down
33 changes: 21 additions & 12 deletions Sources/GravatarUI/SwiftUI/AvatarPicker/AvatarAction.swift
Original file line number Diff line number Diff line change
@@ -1,25 +1,23 @@
import Foundation
import SwiftUI

enum AvatarAction: String, CaseIterable, Identifiable {
enum AvatarAction: Identifiable {
case share
case delete
case rating(AvatarRating)
case playground
case altText

static var allCases: [AvatarAction] {
var cases: [AvatarAction] = []
if #available(iOS 18.2, *) {
if EnvironmentValues().supportsImagePlayground {
cases.append(.playground)
}
var id: String {
switch self {
case .share: "share"
case .delete: "delete"
case .rating(let rating): rating.rawValue
case .playground: "playground"
case .altText: "altText"
}
cases.append(contentsOf: [.share, .altText, .delete])
return cases
}

var id: String { rawValue }

var icon: Image {
switch self {
case .delete:
Expand All @@ -30,6 +28,8 @@ enum AvatarAction: String, CaseIterable, Identifiable {
Image(systemName: "apple.image.playground")
case .altText:
Image(systemName: "text.below.photo")
case .rating:
Image(systemName: "star.leadinghalf.filled")
}
}

Expand Down Expand Up @@ -59,14 +59,23 @@ enum AvatarAction: String, CaseIterable, Identifiable {
value: "Alt Text",
comment: "An option in the avatar menu that edits the avatar's Alt Text."
)
case .rating(let rating):
String(
format: SDKLocalizedString(
"AvatarPicker.AvatarAction.rate",
value: "Rating: %@",
comment: "An option in the avatar menu that shows the current rating, and allows the user to change that rating. The rating is used to indicate the appropriateness of an avatar for different audiences, and follows the US system of Motion Picture ratings: G, PG, R, and X."
),
rating.rawValue
)
}
}

var role: ButtonRole? {
switch self {
case .delete:
.destructive
case .share, .playground, .altText:
case .share, .rating, .playground, .altText:
nil
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ struct AvatarImageModel: Hashable, Identifiable, Sendable {
let isSelected: Bool
let state: State
let altText: String?
let rating: AvatarRating

var url: URL? {
guard case .remote(let url) = source else {
Expand Down Expand Up @@ -47,11 +48,12 @@ struct AvatarImageModel: Hashable, Identifiable, Sendable {
return image
}

init(id: String, source: Source, state: State = .loaded, isSelected: Bool = false, altText: String? = nil) {
init(id: String, source: Source, state: State = .loaded, isSelected: Bool = false, rating: AvatarRating = .g, altText: String? = nil) {
self.id = id
self.source = source
self.state = state
self.isSelected = isSelected
self.rating = rating
self.altText = altText
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,10 @@ struct AvatarPickerView<ImageEditor: ImageEditorView>: View {
}
case .altText:
editAltText(for: avatar)
case .rating(let rating):
Task {
await model.update(avatar, rating: rating)
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ class AvatarPickerViewModel: ObservableObject {

let localID = UUID().uuidString

let localImageModel = AvatarImageModel(id: localID, source: .local(image: image), state: .loading)
let localImageModel = AvatarImageModel(id: localID, source: .local(image: image), state: .loading, rating: .g)
grid.append(localImageModel)

await doUpload(squareImage: image, localID: localID, accessToken: authToken)
Expand Down Expand Up @@ -290,7 +290,8 @@ class AvatarPickerViewModel: ObservableObject {
let newModel = AvatarImageModel(
id: imageID,
source: .local(image: squareImage),
state: .error(supportsRetry: supportsRetry, errorMessage: errorMessage)
state: .error(supportsRetry: supportsRetry, errorMessage: errorMessage),
rating: grid.model(with: imageID)?.rating ?? .g
)
grid.replaceModel(withID: imageID, with: newModel)
}
Expand Down Expand Up @@ -337,6 +338,7 @@ class AvatarPickerViewModel: ObservableObject {
await profile
}

@discardableResult
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reasonable? Or do we want to check the result?

func update(_ avatar: AvatarImageModel, altText: String) async -> Bool {
guard let token = self.authToken else { return false }
do {
Expand All @@ -359,6 +361,35 @@ class AvatarPickerViewModel: ObservableObject {
return false
}

@discardableResult
func update(_ avatar: AvatarImageModel, rating: AvatarRating) async -> Bool {
guard let authToken else { return false }

do {
let updatedAvatar = try await avatarService.update(
nil,
rating: rating,
avatarID: avatar.id,
accessToken: authToken
)
withAnimation {
grid.replaceModel(withID: avatar.id, with: .init(with: updatedAvatar))
toastManager.showToast(Localized.avatarRatingUpdateSuccess, type: .info)
}
return true
} catch APIError.responseError(let reason) where reason.urlSessionErrorLocalizedDescription != nil {
handleError(message: reason.urlSessionErrorLocalizedDescription ?? Localized.avatarRatingError)
} catch {
handleError(message: Localized.avatarRatingError)
}

func handleError(message: String) {
toastManager.showToast(message, type: .error)
}

return false
}

func delete(_ avatar: AvatarImageModel) async -> Bool {
guard let token = self.authToken else { return false }
defer {
Expand Down Expand Up @@ -446,6 +477,16 @@ extension AvatarPickerViewModel {
value: "Image alt text was changed successfully",
comment: "This confirmation message shows when the user has updated the alt text."
)
static let avatarRatingUpdateSuccess = SDKLocalizedString(
"AvatarPickerViewModel.RatingUpdate.Success",
value: "Avatar rating was changed successfully.",
comment: "This confirmation message shows when the user picks a different avatar rating and the change was applied successfully."
)
static let avatarRatingError = SDKLocalizedString(
"AvatarPickerViewModel.Rating.Error",
value: "Oops, something didn't quite work out while trying to rate your avatar.",
comment: "This error message shows when the user attempts to change the rating of an avatar and fails."
)
}
}

Expand All @@ -468,5 +509,6 @@ extension AvatarImageModel {
state = .loaded
isSelected = avatar.isSelected
altText = avatar.altText
rating = avatar.avatarRating
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,25 +80,105 @@ struct AvatarPickerAvatarView: View {

func actionsMenu() -> some View {
Menu {
ForEach(AvatarAction.allCases) { action in
Button(role: action.role) {
onActionTap(action)
} label: {
Label {
Text(action.localizedTitle)
} icon: {
action.icon
Section {
button(for: .share)
if #available(iOS 18.2, *) {
if EnvironmentValues().supportsImagePlayground {
button(for: .playground)
}
}
}
Section {
button(for: .altText)
Menu {
ForEach(AvatarRating.allCases, id: \.self) { rating in
button(for: .rating(rating), isSelected: rating == avatar.rating)
}
} label: {
label(forAction: AvatarAction.rating(avatar.rating))
}
}
Section {
button(for: .delete)
}
} label: {
ellipsisView()
}
}

private func button(
for action: AvatarAction,
isSelected selected: Bool = false,
systemImageWhenSelected systemImage: String = "checkmark"
) -> some View {
Button(role: action.role) {
onActionTap(action)
} label: {
switch action {
case .rating(let rating):
let buttonTitle = "\(rating.rawValue) (\(rating.localizedSubtitle))"

if selected {
label(forAction: action, title: buttonTitle, systemImage: systemImage)
} else {
Text(buttonTitle)
}
case .altText, .delete, .playground, .share:
label(forAction: action)
}
}
}

private func label(forAction action: AvatarAction, title: String? = nil, systemImage: String) -> Label<Text, Image> {
label(forAction: action, title: title, image: Image(systemName: systemImage))
}

private func label(forAction action: AvatarAction, title: String? = nil, image: Image? = nil) -> Label<Text, Image> {
Label {
Text(title ?? action.localizedTitle)
} icon: {
image ?? action.icon
}
}
}

extension AvatarRating {
fileprivate var localizedSubtitle: String {
switch self {
case .g:
SDKLocalizedString(
"Avatar.Rating.G.subtitle",
value: "General",
comment: "Rating that indicates that the avatar is suitable for everyone"
)
case .pg:
SDKLocalizedString(
"Avatar.Rating.PG.subtitle",
value: "Parental Guidance",
comment: "Rating that indicates that the avatar may not be suitable for children"
)
case .r:
SDKLocalizedString(
"Avatar.Rating.R.subtitle",
value: "Restricted",
comment: "Rating that indicates that the avatar may not be suitable for children"
)
case .x:
SDKLocalizedString(
"Avatar.Rating.X.subtitle",
value: "Extreme",
comment: "Rating that indicates that the avatar is obviously and extremely unsuitable for children"
)
}
}
}

#Preview {
let avatar = AvatarImageModel(id: "1", source: .remote(url: "https://gravatar.com/userimage/110207384/aa5f129a2ec75162cee9a1f0c472356a.jpeg?size=256"))
let avatar = AvatarImageModel(
id: "1",
source: .remote(url: "https://gravatar.com/userimage/110207384/aa5f129a2ec75162cee9a1f0c472356a.jpeg?size=256"),
rating: .pg
)
return AvatarPickerAvatarView(avatar: avatar, maxLength: AvatarGridConstants.maxAvatarWidth, minLength: AvatarGridConstants.minAvatarWidth) {
false
} onAvatarTap: { _ in
Expand Down
4 changes: 4 additions & 0 deletions Sources/TestHelpers/Bundle+ResourceBundle.swift
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ extension Bundle {
testsBundle.jsonData(forResource: "avatarUploadResponse")
}

public static var setRatingJsonData: Data {
testsBundle.jsonData(forResource: "avatarSetRatingResponse")
}

public static var getAvatarsJsonData: Data {
testsBundle.jsonData(forResource: "avatarsResponse")
}
Expand Down
7 changes: 7 additions & 0 deletions Sources/TestHelpers/Resources/avatarSetRatingResponse.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"image_id": "991a7b71cf9f34...",
"image_url": "https://2.gravatar.com/userimage/1/991a7b71cf934ea2...?size=512",
"rating": "PG",
"updated_date": "2024-09-18T10:17:53Z",
"alt_text": "John Appleseed's avatar"
}
Loading