appengine/findit/model/crash/cracas_crash_analysis.py - Issue 2523343002: [Predator] Refactor ToPublishResult and fix keyerror 'found'

Unified Diff: appengine/findit/model/crash/cracas_crash_analysis.py

Issue 2523343002: [Predator] Refactor ToPublishResult and fix keyerror 'found' (Closed)

Patch Set: . Created 4 years, 1 month ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« no previous file with comments | « appengine/findit/crash/test/crash_pipeline_test.py ('k') | appengine/findit/model/crash/crash_analysis.py » ('j') | appengine/findit/model/crash/crash_analysis.py » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: appengine/findit/model/crash/cracas_crash_analysis.py

diff --git a/appengine/findit/model/crash/cracas_crash_analysis.py b/appengine/findit/model/crash/cracas_crash_analysis.py

index d57a92524b00a7bb581973a3d6d9efb625f0c2f1..700066ba1ecc0f645014495a7de8a2dc03e71d1a 100644

--- a/appengine/findit/model/crash/cracas_crash_analysis.py

+++ b/appengine/findit/model/crash/cracas_crash_analysis.py

@@ -9,4 +9,8 @@ from model.crash.chrome_crash_analysis import ChromeCrashAnalysis

class CracasCrashAnalysis(ChromeCrashAnalysis):

"""Represents an analysis of a Chrome crash on Cracas."""

- pass

+ def ProcessResultForPublishing(self, result): # pragma: no cover.

+ """Cracas specific processing of result data for publishing."""

+ # TODO(katesonia) Add feedback page for Cracas after Cracas integration.

stgao 2016/11/23 23:51:14 Why it is to be added here? Shouldn't model just t

Sharu Jiang 2016/11/24 00:13:38 The previous ``ToPublishResult`` is in the publish

stgao 2016/11/29 18:44:35 This is good. I don't have concern on this.

Sharu Jiang 2016/11/29 20:49:43 Ok, sorry, I misunderstood your comment. Ｔhe url t

stgao 2016/11/30 04:35:07 So where is the URL from/defined? How the code sho

Sharu Jiang 2016/11/30 18:14:07 If this is about where we should insert the feedba

If this is about where we should insert the feedback url to result for publishing: I have concern mentioned in the thread, if we move this part to crash_pipeline we would have if client == FRACAS..., which I think it's the thing we want to avoid. If we move it to FinditFor*, it will be a little wired that we have some processing in CrashAnalysis model and some in FinditFor... class. I am a little confused, this may deal with url but it is one kind of processing just as ``ToPublishableResult`` in CrashAnalysis. If we are to move it, where should this function stay? If this is all about the TODO: I rephrased it and hope it's more clear that this function won't deal with the url but just insert this url to result.

stgao 2016/11/30 18:56:06 The wording is fine, but my concern is what is to

On 2016/11/30 18:14:07, Sharu Jiang wrote: > On 2016/11/30 04:35:07, stgao (slow on Monday) wrote: > > On 2016/11/29 20:49:43, Sharu Jiang wrote: > > > On 2016/11/29 18:44:35, stgao (slow on Monday) wrote: > > > > On 2016/11/24 00:13:38, Sharu Jiang wrote: > > > > > On 2016/11/23 23:51:14, stgao (slow on Monday) wrote: > > > > > > Why it is to be added here? Shouldn't model just take care of data > > > > > > saving/updating/retrieving? Why we want it to deal with UI layer (url > to > > > > > > feedback page)? > > > > > > > > > > The previous ``ToPublishResult`` is in the publish pipeline, however it > > has > > > > been > > > > > moved to CrashAnalysis in that major refactor, I thought you agreed on > > this > > > > > refactor? > > > > > > > > This is good. I don't have concern on this. > > > > > > > > > > > > > > I just split the client specific part in ToPublishResult to subclasses. > > > > > > > > > > If you'd prefer, I can move the ``ToPublishResult`` back to publish > > pipeline > > > > and > > > > > let FinditFor* to deal with this processing. > > > > > > > > My concern is the TODO added here. That's why the comment is at line #15 > > > instead > > > > of line #13, and the wording is "to be added". > > > > > > > > > > Ok, sorry, I misunderstood your comment. Ｔhe url to feedback link is like > meta > > > data added to the model result to publish to client. I don't think it > actually > > > dealt with the UI layer． > > > > So where is the URL from/defined? How the code should be split into the MVC > > design pattern and the dependency among them? > > If this is about where we should insert the feedback url to result for > publishing: > > I have concern mentioned in the thread, if we move this part to crash_pipeline > we would have if > client == FRACAS..., which I think it's the thing we want to avoid. If we move > it to FinditFor*, it will be a little wired that we have some processing in > CrashAnalysis model and some in FinditFor... class. > > I am a little confused, this may deal with url but it is one kind of processing > just as ``ToPublishableResult`` in CrashAnalysis. If we are to move it, where > should this function stay? > > If this is all about the TODO: > I rephrased it and hope it's more clear that this function won't deal with the > url but just insert this url to result.

The wording is fine, but my concern is what is to be done. To be more explicit, I have a question here: why the url defined for a handler in the controller layer is added to the model layer as in model/crash/fracas_crash_analysis.py below? Isn't it a violation of the MVC pattern? Should this be fixed? Are there alternatives to add the url from the controller layer like the publish pipeline?

Sharu Jiang 2016/11/30 19:26:05 I think move all the ToPublishableResult to Findit

On 2016/11/30 18:56:06, stgao (slow on Monday) wrote: > On 2016/11/30 18:14:07, Sharu Jiang wrote: > > On 2016/11/30 04:35:07, stgao (slow on Monday) wrote: > > > On 2016/11/29 20:49:43, Sharu Jiang wrote: > > > > On 2016/11/29 18:44:35, stgao (slow on Monday) wrote: > > > > > On 2016/11/24 00:13:38, Sharu Jiang wrote: > > > > > > On 2016/11/23 23:51:14, stgao (slow on Monday) wrote: > > > > > > > Why it is to be added here? Shouldn't model just take care of data > > > > > > > saving/updating/retrieving? Why we want it to deal with UI layer > (url > > to > > > > > > > feedback page)? > > > > > > > > > > > > The previous ``ToPublishResult`` is in the publish pipeline, however > it > > > has > > > > > been > > > > > > moved to CrashAnalysis in that major refactor, I thought you agreed on > > > this > > > > > > refactor? > > > > > > > > > > This is good. I don't have concern on this. > > > > > > > > > > > > > > > > > I just split the client specific part in ToPublishResult to > subclasses. > > > > > > > > > > > > If you'd prefer, I can move the ``ToPublishResult`` back to publish > > > pipeline > > > > > and > > > > > > let FinditFor* to deal with this processing. > > > > > > > > > > My concern is the TODO added here. That's why the comment is at line #15 > > > > instead > > > > > of line #13, and the wording is "to be added". > > > > > > > > > > > > > Ok, sorry, I misunderstood your comment. Ｔhe url to feedback link is like > > meta > > > > data added to the model result to publish to client. I don't think it > > actually > > > > dealt with the UI layer． > > > > > > So where is the URL from/defined? How the code should be split into the MVC > > > design pattern and the dependency among them? > > > > If this is about where we should insert the feedback url to result for > > publishing: > > > > I have concern mentioned in the thread, if we move this part to crash_pipeline > > we would have if > > client == FRACAS..., which I think it's the thing we want to avoid. If we move > > it to FinditFor*, it will be a little wired that we have some processing in > > CrashAnalysis model and some in FinditFor... class. > > > > I am a little confused, this may deal with url but it is one kind of > processing > > just as ``ToPublishableResult`` in CrashAnalysis. If we are to move it, where > > should this function stay? > > > > If this is all about the TODO: > > I rephrased it and hope it's more clear that this function won't deal with the > > url but just insert this url to result. > > The wording is fine, but my concern is what is to be done. > > To be more explicit, I have a question here: why the url defined for a handler > in the controller layer is added to the model layer as in > model/crash/fracas_crash_analysis.py below? Isn't it a violation of the MVC > pattern? Should this be fixed? > > Are there alternatives to add the url from the controller layer like the publish > pipeline?

I think move all the ToPublishableResult to FinditFor* can solve all concerns. If we all agree on this, I will go ahead and move it.

+ return result