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

Adding ability to send IP address from API routes #119

Closed
wants to merge 6 commits into from
Closed

Adding ability to send IP address from API routes #119

wants to merge 6 commits into from

Conversation

Xoshbin
Copy link

@Xoshbin Xoshbin commented Nov 11, 2018

Fixes #82

If you are using this library as a backend server for native applications like Android or IOS you can send the IP address from the application through HTTP request like below:
$post->addView($ip);

@codecov
Copy link

codecov bot commented Nov 11, 2018

Codecov Report

Merging #119 into 2.x will decrease coverage by 0.35%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x     #119      +/-   ##
============================================
- Coverage     98.51%   98.16%   -0.36%     
- Complexity      105      106       +1     
============================================
  Files            11       11              
  Lines           270      272       +2     
============================================
+ Hits            266      267       +1     
- Misses            4        5       +1
Impacted Files Coverage Δ Complexity Δ
src/ViewableService.php 100% <100%> (ø) 30 <7> (ø) ⬇️
src/Viewable.php 100% <100%> (ø) 9 <1> (ø) ⬇️
src/Support/IpAddress.php 75% <75%> (-25%) 2 <2> (+1)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03c2e6a...c8958ca. Read the comment docs.

@@ -49,7 +49,7 @@ public function getUniqueViewsCount($viewable, $period = null): int;
* @param \Illuminate\Database\Eloquent\Model $viewable
* @return bool
*/
public function addViewTo($viewable): bool;
public function addViewTo($viewable, $ip = ''): bool;
Copy link
Owner

Choose a reason for hiding this comment

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

While I don't have any plans for chaning this methods signature in v2.x, I think it's better to replace $ip = '' with $properties = [] or $data = [], so if we want to add more options, they can pass it inside the same array instead of ...->addViewTo($post, false, $someVar, $someOtherVar).

@@ -28,8 +28,12 @@ class IpAddress
*
* @return bool
*/
public function get()
public function get($ip = '')
Copy link
Owner

Choose a reason for hiding this comment

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

No need to change this class.

@@ -73,9 +73,9 @@ public function getUniqueViews($period = null) : int
*
* @return bool
*/
public function addView(): bool
public function addView($ip = ''): bool
Copy link
Owner

Choose a reason for hiding this comment

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

See first comment.

@@ -190,12 +190,12 @@ public function addViewTo($viewable): bool

$ignoredIpAddresses = Collection::make(config('eloquent-viewable.ignored_ip_addresses', []));

if ($ignoredIpAddresses->contains($this->ipRepository->get())) {
if ($ignoredIpAddresses->contains($this->ipRepository->get($ip))) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can do: $properties['ip'] ?? $this->ipRepository->get()

return false;
}

$visitorCookie = Cookie::get($cookieName);
$visitor = $visitorCookie ?? $this->ipRepository->get();
$visitor = $visitorCookie ?? $this->ipRepository->get($ip);
Copy link
Owner

Choose a reason for hiding this comment

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

Same: $properties['ip'] ?? $this->ipRepository->get()

@cyrildewit cyrildewit changed the base branch from master to 2.x November 16, 2018 09:33
@cyrildewit cyrildewit modified the milestones: Release v2.x, Release 3.0 Nov 27, 2018
@cyrildewit cyrildewit closed this Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants