Skip to content

Jetpack Mobile: Threat Details - custom views#13704

Merged
zwarm merged 4 commits intodevelopfrom
issue/13326-threat-details-custom-views
Jan 5, 2021
Merged

Jetpack Mobile: Threat Details - custom views#13704
zwarm merged 4 commits intodevelopfrom
issue/13326-threat-details-custom-views

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Jan 4, 2021 •
edited
Loading

Parent: #13703

This PR creates custom ViewTypes for ThreatContext and FileName for "Threat Details" screen.

References: Scan design iteration i5

device-2021-01-04-131412

To test

Prerequisite:

  1. Make sure both Scan feature flag and MySiteImprovements flag are set to on in the App settings.
  2. WP.com account with a site having scan capability and different threats (e.g. http://do.wpmt.co/jp-scan-daily/).
  • Login to the app using above WP.com account and select the site on the My Site tab
  • Notice that scan menu is displayed
  • Click scan menu item followed by threat item
  • Notice that ThreatContext.lines are shown correctly with a highlighted line
  • ViewType for ThreatFileName can be tested by putting below line above this line in ThreatDetailsListItemsBuilder:
    threatModel.fileName?.let { items.add(ThreatFileNameState(UiStringText(it))) }

Notes

  1. Ignore all styling
  2. Tests covered in parent PR Jetpack Mobile: Threat Details - list items builder #13703

Merge Instructions

  1. Make sure Threat Details: Skeleton with blank Content UiState #13675 is merged
  2. Remove the "Not Ready for Merge label"
  3. Merge this PR

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@ashiagr ashiagr added this to the 16.5 milestone Jan 4, 2021
@ashiagr ashiagr self-assigned this Jan 4, 2021
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 4, 2021 •
edited
Loading

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 4, 2021 •
edited
Loading

You can test the changes on this Pull Request by downloading the APK here.

@ashiagr ashiagr mentioned this pull request Jan 4, 2021
Merged
3 tasks
@ashiagr ashiagr marked this pull request as ready for review January 4, 2021 10:31
@ashiagr ashiagr requested a review from zwarm January 4, 2021 10:31
zwarm
zwarm reviewed Jan 4, 2021
View reviewed changes
WordPress/src/main/java/org/wordpress/android/ui/jetpack/common/ViewType.kt Outdated
BACKUP_SUB_HEADER(7)
BACKUP_SUB_HEADER(7),
THREAT_CONTEXT_LINES(8),
FILE_NAME(9)
Copy link
Copy Markdown
Contributor

@zwarm zwarm Jan 4, 2021

Choose a reason for hiding this comment

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

@ashiagr - Is FILE_NAME is exclusive to the scan feature and if so, should we prefix it as such like we did for the others (backup, threats)? wdyt?

Copy link
Copy Markdown
Contributor Author

@ashiagr ashiagr Jan 5, 2021

Choose a reason for hiding this comment

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

Good point. Renamed in aa33ca5.

zwarm
zwarm reviewed Jan 4, 2021
View reviewed changes
WordPress/src/main/java/org/wordpress/android/ui/utils/PaddingBackgroundColorSpan.kt

/**
* Creates BackgroundColorSpan with padding
* Credits: https://medium.com/@tokudu/android-adding-padding-to-backgroundcolorspan-179ab4fae187
Copy link
Copy Markdown
Contributor

@zwarm zwarm Jan 4, 2021

Choose a reason for hiding this comment

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

@ashiagr - I never noticed the "non" spacing before, but this is a good hack. Nice find and enhancement

zwarm
zwarm reviewed Jan 4, 2021
View reviewed changes
...src/main/java/org/wordpress/android/ui/jetpack/scan/details/ThreatDetailsListItemsBuilder.kt

@Reusable
class ThreatDetailsListItemsBuilder @Inject constructor() {
// TODO ashiagr to be implemented
Copy link
Copy Markdown
Contributor

@zwarm zwarm Jan 4, 2021

Choose a reason for hiding this comment

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

Tiny nitpick - Looks like you implemented this, do you want to keep the TODO any longer?

Copy link
Copy Markdown
Contributor Author

@ashiagr ashiagr Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is not fully implemented in this PR. I've already removed it in the parent PR #13703 here.

@zwarm zwarm self-assigned this Jan 4, 2021
@zwarm
Copy link
Copy Markdown
Contributor

zwarm commented Jan 4, 2021

@ashiagr - So far the code looks good, but I need your help (or a point in the right direction) for

Select a site (http://do.wpmt.co/jp-scan-daily/) with scan capability and a threat with "malicious code pattern" on the My Site tab

I have no idea how to set that up. Help please. Thanks

@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Jan 5, 2021

Thanks @zwarm! I've addressed your comments so far.

So far the code looks good, but I need your help (or a point in the right direction)
I have no idea how to set that up. Help please. Thanks

Pinged you on Slack with instructions.

Base automatically changed from issue/13326-threat-details-skeleton-and-blank-content-uistate to develop January 5, 2021 07:17
zwarm
zwarm approved these changes Jan 5, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@zwarm zwarm left a comment

Choose a reason for hiding this comment

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

LGTM! Highlighted as expected. Nicely done. 👍

@zwarm zwarm merged commit 0259a9b into develop Jan 5, 2021
@zwarm zwarm deleted the issue/13326-threat-details-custom-views branch January 5, 2021 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zwarm zwarm zwarm approved these changes

Assignees

@zwarm zwarm

@ashiagr ashiagr

Labels

[Type] Task

Projects

None yet

Milestone

16.5

Development

Successfully merging this pull request may close these issues.

2 participants

@ashiagr @zwarm