Fixes #724: Push search data to server using api/push.json endpoint of server #753

simsausaurabh · 2018-06-14T16:38:13Z

Changes proposed in this pull request

Created a new Push Service to make a Post request.
Created a new Action to call push service when there is a successful SeachAction (When search is successful).

Screenshots (if appropriate)

Link to live demo: http://pr-753-fossasia-loklaksearch.surge.sh

Closes #724

praveenojha33

Please look why Travis is failing.

codecov · 2018-06-14T19:06:48Z

Codecov Report

Merging #753 into development will increase coverage by 0.01%.
The diff coverage is 46.15%.

@@               Coverage Diff               @@
##           development     #753      +/-   ##
===============================================
+ Coverage        51.66%   51.67%   +0.01%     
===============================================
  Files               99      100       +1     
  Lines             2499     2512      +13     
  Branches           278      278              
===============================================
+ Hits              1291     1298       +7     
- Misses            1180     1187       +7     
+ Partials            28       27       -1

Impacted Files	Coverage Δ
src/app/models/index.ts	`100% <ø> (ø)`	⬆️
src/app/services/push.service.ts	`46.15% <46.15%> (ø)`
src/app/lazy-img/lazy-img.service.ts	`64.44% <0%> (+2.22%)`	⬆️

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 59c45b7...d33b21a. Read the comment docs.

simsausaurabh · 2018-06-14T19:22:23Z

@praveenojha33 I have fixed it, please review again.

@singhpratyush @sudheesh001 Please review. It is made for pushing one status at a time for now. I am working to make it parallel using something like fork_join. But it will take sometime, till then it can be used to boost up server's data. Sending more than one post request with more than one status leads to server error for request.

singhpratyush · 2018-06-15T08:25:40Z

@simsausaurabh: What error were you facing with the request with multiple messages? Can you paste the cURL for the request that you were making?

simsausaurabh · 2018-06-15T08:39:52Z

@singhpratyush I was facing server 503 error while making a request with multiple messages. Currently server is not returning results, so I am unable to provide that snapshot. I am making a post request from the created Push Service. Here: https://github.com/fossasia/loklak_search/pull/753/files#diff-06795d93ddaa297718495e8ade90d648

simsausaurabh · 2018-06-15T08:44:06Z

I have made a separate utility function, and kept the calling limited to only 1. Once I figure out in making multiple request parallelly, it will be increased to 20 (Either 2 at a time in one request which makes only 10 parallel post request, or 1 at a time which will increase the load to 20 times i.e. 20 parallel post request). Because sending more than 2 statuses in one post request is also stating 503 server error.

simsausaurabh · 2018-06-15T12:29:38Z

@singhpratyush Please review

singhpratyush · 2018-06-15T13:25:06Z

src/app/services/push.service.ts

+			'Accept': 'application/json',
+			'cache-control': 'no-cache'
+			});
+		if (data) {


This is a bit confusing. Please add conditionals to explain this.

singhpratyush · 2018-06-15T13:26:04Z

src/app/services/push.service.ts

+			'cache-control': 'no-cache'
+			});
+		if (data) {
+				const temp = '' + JSON.stringify(data);


Makes no sense to add '' to a string, it remains the same.

singhpratyush · 2018-06-15T13:29:30Z

src/app/services/push.service.ts

+				if (JSON.parse(temp)['aggregations']) {
+					const subTemp = temp.substr(0, temp.length - 19) + '}';
+					if (JSON.parse(subTemp)['statuses'][0]) {
+						const metadata = '{"search_metadata":' + getMetadata(subTemp) + ',"statuses":';


Not sure what this is about but this is not the right way to generate JSON strings.

Something as simple as this should do -

const {metadata, statuses} = data; const dataToSend = JSON.stringify({search_metadata: metadata, statuses});

singhpratyush · 2018-06-15T13:30:20Z

src/app/services/push.service.ts

+						const value = metadata + getStatuses(subTemp) + '}';
+						const body = new HttpParams()
+							.set('data', value);
+						this.http.post(jsonpUrl, body.toString(), {headers: headers}).subscribe(val => console.log(val));


Don't we need to dispatch any actions with this event?

We don't need an additional action for this, although we can create one. But what could it be used for? For stating a success on push? We can create one for it, but it would not be used as of now anywhere else. And we can not use already available SEARCH_COMPLETE_SUCCESS which do not favours this context.

If I am missing on any point, then please guide me to it.

singhpratyush · 2018-06-15T13:32:13Z

src/app/services/push.service.spec.ts

+import { HttpClientModule, HttpClient } from '@angular/common/http';
+import { PushService } from './push.service';
+
+describe('PushService', () => {


Tests technically do not test anything significant.

How do you know that proper calls are made and functions are called accordingly? Please try to cover more cases.

I tried to use already existing mock data inside shared/mocks/feedItem.mock.ts, but there comes some issue with some of the parameters inside individual: search_metadata, statuses, and aggregations. I tried to make it work out, but it is taking longer time than expected. Currently the simple test checks that push service is being created. I will add tests in later patches (Because many other parts need new tests since the project is upgraded, and the tests written earlier can't be taken as it is, I have to write new ones). I want to fix sidebar, newsTab issue firstly before this weekend. Hope that works?

singhpratyush · 2018-06-15T13:34:49Z

src/app/effects/api-search.effects.ts

@@ -118,6 +118,7 @@ export class ApiSearchEffects {
 				),
 				withLatestFrom(this.store$),
 				map(([action, state]) => {
+					this.pushService.postData(action['payload']);


It is otiose to push data to the backend when you are actually getting the data from the backend. It would already contain all the statuses in this case.

The push servlet is supposed to be used when we are fetching Tweets using Twitter API they may be new for the backend.

Yes, I agree it would be more helpful once we complete the twitter api support. But till then it could work I guess, after that we can just push the mapped data from twitter api i.e. the data extracted from twitter api mapped in format of api.loklak. I have made all other changes, and I will surely write the tests in further patches. Thanks @singhpratyush.

singhpratyush · 2018-06-15T13:36:11Z

src/app/utils/push.ts

@@ -0,0 +1,7 @@
+export function getStatuses(data: string): string {


I'm not sure why you are jumping forth and back with object and JSON string. This can be made very simple. Just use JSON.stringify when needed.

simsausaurabh · 2018-06-15T21:41:41Z

@singhpratyush Please review. I will write test in further patches, since its taking longer time than expected and there are issues like newsTab and sidebar which needs to be fixed before this weekend.

singhpratyush · 2018-06-19T05:42:55Z

src/app/services/push.service.ts

+
+	constructor( private http: HttpClient ) { }
+
+	public postData(data: ApiResponse) {


Shouldn't this method use some callback or return a promise? How will other parts of the code interact with the completion state of this method?

I guess we do not need a call back or a return promise since we are not currently using the returned response apart from logging it on console.

But surely, we can return the response. It can be used to test the method/push service, and I would be adding it while writing test case for it.

simsausaurabh · 2018-06-19T07:27:15Z

@singhpratyush Please review. I am writing tests. I will be sending them frequently in further patches.

singhpratyush

Style issues are not leading for travis build to fail, not sure why.

Almost good to merge, please look at the comments.

singhpratyush · 2018-06-19T10:06:56Z

src/app/services/push.service.ts

+		});
+		// Extracting search_metadata and statuses
+		// from the data obtained from search result.
+		const {search_metadata, statuses} = data;


We're using lowerCamelCase, please be consistent.

Not sure why the build isn't failing for this.

But search_metadata is the property inside data. It shows error removing/making it to lowerCameCase, since it would not be a know property of data.

const { statuses } = data; const searchMetadata = data.search_metadata;

This becomes long, but consistency is maintained. We can't control search_metadata in response, but we can control what we want to use as a variable.

singhpratyush · 2018-06-19T10:08:04Z

src/app/services/push.service.ts

+		// Making a Post request to api/push.json endpoint
+		// of server with required header and data body.
+		// Response Object is converted to PushApiResponse type.
+		return this.http.post<PushApiResponse>(httpUrl, body, {headers: headers});


{headers} would do.

simsausaurabh · 2018-06-19T10:57:12Z

@singhpratyush Please review.

simsausaurabh · 2018-06-19T11:07:25Z

@singhpratyush Please review.

simsausaurabh · 2018-06-19T11:12:29Z

@mariobehling Please review.

praveenojha33 suggested changes Jun 14, 2018

View reviewed changes

simsausaurabh force-pushed the pushapi branch from 9dd358e to 9e3af0f Compare June 14, 2018 19:02

simsausaurabh force-pushed the pushapi branch from 9e3af0f to 6c35af5 Compare June 15, 2018 12:26

singhpratyush suggested changes Jun 15, 2018

View reviewed changes

simsausaurabh force-pushed the pushapi branch 2 times, most recently from a393644 to c40509f Compare June 15, 2018 21:39

singhpratyush reviewed Jun 19, 2018

View reviewed changes

simsausaurabh force-pushed the pushapi branch 2 times, most recently from 6bf5769 to cc4f8c8 Compare June 19, 2018 07:24

singhpratyush suggested changes Jun 19, 2018

View reviewed changes

simsausaurabh force-pushed the pushapi branch from a74e8aa to acde62f Compare June 19, 2018 10:36

Fixes fossasia#724: Push search data to server using api/push.json

85978ed

simsausaurabh force-pushed the pushapi branch from 8438050 to 85978ed Compare June 19, 2018 11:04

Merge branch 'development' into pushapi

d33b21a

singhpratyush approved these changes Jun 19, 2018

View reviewed changes

praveenojha33 approved these changes Jun 19, 2018

View reviewed changes

mariobehling merged commit cb39145 into fossasia:development Jun 20, 2018

simsausaurabh deleted the pushapi branch June 20, 2018 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #724: Push search data to server using api/push.json endpoint of server #753

Fixes #724: Push search data to server using api/push.json endpoint of server #753

simsausaurabh commented Jun 14, 2018 •

edited

praveenojha33 left a comment

codecov bot commented Jun 14, 2018 •

edited

simsausaurabh commented Jun 14, 2018 •

edited

singhpratyush commented Jun 15, 2018

simsausaurabh commented Jun 15, 2018

simsausaurabh commented Jun 15, 2018 •

edited

simsausaurabh commented Jun 15, 2018

singhpratyush Jun 15, 2018

singhpratyush Jun 15, 2018

singhpratyush Jun 15, 2018

singhpratyush Jun 15, 2018

simsausaurabh Jun 15, 2018

simsausaurabh Jun 15, 2018

singhpratyush Jun 15, 2018

simsausaurabh Jun 15, 2018

singhpratyush Jun 15, 2018

simsausaurabh Jun 15, 2018 •

edited

singhpratyush Jun 15, 2018

simsausaurabh commented Jun 15, 2018

singhpratyush Jun 19, 2018

simsausaurabh Jun 19, 2018

simsausaurabh commented Jun 19, 2018

singhpratyush left a comment

singhpratyush Jun 19, 2018

simsausaurabh Jun 19, 2018 •

edited

singhpratyush Jun 19, 2018

singhpratyush Jun 19, 2018

simsausaurabh commented Jun 19, 2018

simsausaurabh commented Jun 19, 2018

simsausaurabh commented Jun 19, 2018

		@@ -0,0 +1,7 @@
		export function getStatuses(data: string): string {


		constructor( private http: HttpClient ) { }

		public postData(data: ApiResponse) {

Fixes #724: Push search data to server using api/push.json endpoint of server #753

Fixes #724: Push search data to server using api/push.json endpoint of server #753

Conversation

simsausaurabh commented Jun 14, 2018 • edited

praveenojha33 left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 14, 2018 • edited

Codecov Report

simsausaurabh commented Jun 14, 2018 • edited

singhpratyush commented Jun 15, 2018

simsausaurabh commented Jun 15, 2018

simsausaurabh commented Jun 15, 2018 • edited

simsausaurabh commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simsausaurabh Jun 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simsausaurabh commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simsausaurabh commented Jun 19, 2018

singhpratyush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simsausaurabh Jun 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simsausaurabh commented Jun 19, 2018

simsausaurabh commented Jun 19, 2018

simsausaurabh commented Jun 19, 2018

simsausaurabh commented Jun 14, 2018 •

edited

codecov bot commented Jun 14, 2018 •

edited

simsausaurabh commented Jun 14, 2018 •

edited

simsausaurabh commented Jun 15, 2018 •

edited

simsausaurabh Jun 15, 2018 •

edited

simsausaurabh Jun 19, 2018 •

edited