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.

Sign up for GitHub

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

Jump to bottom

Ads History should be displayed in reverse chronological order #4109

Merged
merged 8 commits into from
Dec 9, 2019
Merged

Ads History should be displayed in reverse chronological order #4109

merged 8 commits into from
Dec 9, 2019

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Nov 30, 2019

Fixes brave/brave-browser#6757

Submitter Checklist:

Test Plan:

See brave/brave-browser#6757

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@tmancey tmancey added this to the 1.3.x - Nightly milestone Nov 30, 2019
@tmancey tmancey self-assigned this Nov 30, 2019
@tmancey tmancey force-pushed the issues/6757 branch 2 times, most recently from 6eb3b6c to 16f13e7 Compare November 30, 2019 20:21
@tmancey tmancey force-pushed the issues/6757 branch 2 times, most recently from bd95911 to 6366e87 Compare December 4, 2019 18:13
@NejcZdovc NejcZdovc removed this from the 1.3.x - Dev milestone Dec 5, 2019
@tmancey tmancey force-pushed the issues/6757 branch 8 times, most recently from 7d9a5a3 to 9866b39 Compare December 5, 2019 20:26
@tmancey tmancey marked this pull request as ready for review December 5, 2019 20:28
kylehickinson
kylehickinson approved these changes Dec 5, 2019
View reviewed changes
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

iOS changes look good

@tmancey tmancey force-pushed the issues/6757 branch 2 times, most recently from 396d7d8 to 1384c2e Compare December 6, 2019 07:38
@tmancey tmancey force-pushed the issues/6757 branch 2 times, most recently from cc167e3 to d7aa2bc Compare December 6, 2019 10:10
@tmancey tmancey added this to the 1.4.x - Nightly milestone Dec 6, 2019
NejcZdovc
NejcZdovc reviewed Dec 9, 2019
View reviewed changes
components/definitions/rewards.d.ts
@@ -253,14 +253,14 @@ declare namespace Rewards {
args: Record<string, string>
}

export interface AdsHistoryData {
export interface AdsHistory {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it's really hard to see a difference between AdsHistory and AdHistory, so maybe revert to AdHistoryDetail

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AdsHistory is the plural version of AdHistory due to needed serialization and deserialization. Prefer to leave the same as they are related

NejcZdovc
NejcZdovc reviewed Dec 9, 2019
View reviewed changes
components/brave_rewards/resources/page/components/adsBox.tsx
let groupedAdsHistory: Rewards.AdsHistory[] = []

for (let i = 0; i < adsHistory.length; i++) {
const adHistory = adsHistory[i]
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be null/undefined? I think it would be good to check it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This cannot be null or undefined

NejcZdovc
NejcZdovc reviewed Dec 9, 2019
View reviewed changes
components/brave_rewards/resources/page/components/adsBox.tsx
const flooredDateString = flooredDate.toLocaleDateString()

for (let j = 0; j < adHistory.adDetailRows.length; j++) {
const adDetailRow = this.getAdDetailRow(adHistory.adDetailRows[j])
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be null/undefined? I think it would be good to check it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This cannot be null or undefined

NejcZdovc
NejcZdovc approved these changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

left some nits. Please check why macOS failed, you can run just CI for macOS as it passed on all other platforms

@tmancey tmancey added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Dec 9, 2019
@tmancey
Copy link
Collaborator Author

tmancey commented Dec 9, 2019

macOS failed for unrelated reasons, discussed with @nejc and agreed to merge

@tmancey tmancey merged commit 33f6998 into master Dec 9, 2019
@tmancey tmancey deleted the issues/6757 branch December 9, 2019 10:36
@tmancey tmancey removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux PR/Pending Review labels Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gdregalo gdregalo gdregalo approved these changes

@masparrow masparrow masparrow approved these changes

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@kylehickinson kylehickinson kylehickinson approved these changes

@emerick emerick Awaiting requested review from emerick

@ryanml ryanml Awaiting requested review from ryanml

Assignees

@tmancey tmancey

Labels
feature/ads
Projects
None yet
Milestone
1.4.x - Release
Development

Successfully merging this pull request may close these issues.

Ads History should be displayed in reverse chronological order
5 participants
@tmancey @kylehickinson @NejcZdovc @masparrow @gdregalo