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

Add Origins page Traffic Portal v2 #7881

Merged
merged 8 commits into from
Feb 7, 2024
Merged

Conversation

ntheanh201
Copy link
Contributor

Origins is currently redirecting to Traffic Portal. So that's why I add Origins to Traffic Portal v2


Which Traffic Control components are affected by this PR?

  • Traffic Portal V2

What is the best way to verify this PR?

  • TPv2 has it own Origins page

If this is a bugfix, which Traffic Control versions contained the bug?

PR submission checklist

@ntheanh201 ntheanh201 marked this pull request as draft December 5, 2023 10:18
Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (96d300e) 29.06% compared to head (7b7e33a) 65.78%.
Report is 5 commits behind head on master.

Files Patch % Lines
...app/core/origins/detail/origin-detail.component.ts 68.18% 21 Missing ⚠️
...affic-portal/src/app/api/testing/origin.service.ts 57.77% 18 Missing and 1 partial ⚠️
...c-portal/src/app/api/testing/coordinate.service.ts 28.57% 10 Missing ⚠️
.../app/core/origins/table/origins-table.component.ts 97.14% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             master    #7881       +/-   ##
=============================================
+ Coverage     29.06%   65.78%   +36.71%     
  Complexity       98       98               
=============================================
  Files           605      329      -276     
  Lines         77535    13038    -64497     
  Branches         90     1001      +911     
=============================================
- Hits          22539     8577    -13962     
+ Misses        52908     4100    -48808     
+ Partials       2088      361     -1727     
Flag Coverage Δ
golib_unit ?
grove_unit ?
t3c_unit ?
traffic_monitor_unit ?
traffic_ops_unit ?
traffic_portal_v2 74.37% <74.24%> (?)
traffic_stats_unit ?
unit_tests 74.37% <74.24%> (+48.45%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

I'm assuming you opened this PR to look for feedback, even though it's still a draft. I realize parts of it aren't ready for that yet, but I commented on them a couple times anyway, just in case. The table controller looks mostly good, although I haven't compared it to the functionality of the TPv1 counterpart. Obviously testing needs a lot of work.
I'm gonna try to work on migrating our tests to cypress over the holidays, which will hopefully make the e2e testing easier.

@ntheanh201
Copy link
Contributor Author

I'm assuming you opened this PR to look for feedback, even though it's still a draft. I realize parts of it aren't ready for that yet, but I commented on them a couple times anyway, just in case. The table controller looks mostly good, although I haven't compared it to the functionality of the TPv1 counterpart. Obviously testing needs a lot of work. I'm gonna try to work on migrating our tests to cypress over the holidays, which will hopefully make the e2e testing easier.

Yeah, I made it in draft because it's not ready for review yet. But thanks for your review, I'll check it. The code is copied from some other components and some parts I didn't change it ^^

@ntheanh201 ntheanh201 marked this pull request as ready for review December 15, 2023 08:17
@ntheanh201
Copy link
Contributor Author

@ocket8888 Can you take a look?

Copy link
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

I do also still have to check the coverage; need to make sure it hits 100% in the modified files.

@ocket8888 ocket8888 added new feature A new feature, capability or behavior low impact affects only a small portion of a CDN, and cannot itself break one experimental a feature/component not directly supported by ATC Traffic Portal v2 Related to the experimental Traffic Portal version 2 labels Jan 8, 2024
@ocket8888 ocket8888 self-assigned this Jan 8, 2024
*
* @param e HTML form submission event.
*/
public async submit(e: Event): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is not tested


/** Defines what the table should do when a row is double-clicked. */
public doubleClickLink: DoubleClickLink<RequestOriginResponse> = {
href: (row: RequestOriginResponse): string => `/core/origins/${row.id}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Though it seems pedantic and trivial, testing this object's href method is the only thing preventing this file from having 100% coverage.

@ocket8888 ocket8888 merged commit b7f5774 into apache:master Feb 7, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental a feature/component not directly supported by ATC low impact affects only a small portion of a CDN, and cannot itself break one new feature A new feature, capability or behavior Traffic Portal v2 Related to the experimental Traffic Portal version 2
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants