Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2523343002:
[Predator] Refactor ToPublishResult and fix keyerror 'found' (Closed)
|
Created:
4 years ago by Sharu Jiang Modified:
4 years ago CC:
chromium-reviews, infra-reviews+infra_chromium.org Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[Predator] Refactor ToPublishResult and fix keyerror 'found'
BUG=668303
Committed: https://chromium.googlesource.com/infra/infra/+/d10052ed79ae195e1569f18f52d24990b35f3a4f
Patch Set 1 : . #
Total comments: 18
Patch Set 2 : Fix nits. #Patch Set 3 : Fix nits. #Patch Set 4 : Move ToPublishableResult to Findit* #Patch Set 5 : Fix nits. #Patch Set 6 : Rebase. #
Messages
Total messages: 32 (12 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Predator] Refactor ToPublishResult. BUG=667550 ========== to ========== [Predator] Refactor ToPublishResult. BUG=668303 ==========
katesonia@chromium.org changed reviewers: + stgao@chromium.org, wrengr@chromium.org
Description was changed from ========== [Predator] Refactor ToPublishResult. BUG=668303 ========== to ========== [Predator] Refactor ToPublishResult and fix keyerror 'found' BUG=668303 ==========
Patchset #1 (id:20001) has been deleted
PTAL :)
https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/cracas_crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/cracas_crash_analysis.py:15: # TODO(katesonia) Add feedback page for Cracas after Cracas integration. 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)? https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/fracas_crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/fracas_crash_analysis.py:10: FRACAS_FEEDBACK_URL_TEMPLATE = '%s/crash/fracas-result-feedback?key=%s' See question above.
https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/cracas_crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/cracas_crash_analysis.py:15: # TODO(katesonia) Add feedback page for Cracas after Cracas integration. 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? 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. https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/fracas_crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/fracas_crash_analysis.py:10: FRACAS_FEEDBACK_URL_TEMPLATE = '%s/crash/fracas-result-feedback?key=%s' On 2016/11/23 23:51:14, stgao (slow on Monday) wrote: > See question above. As above.
https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/cracas_crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/cracas_crash_analysis.py:15: # TODO(katesonia) Add feedback page for Cracas after Cracas integration. 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".
https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/cracas_crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/cracas_crash_analysis.py:15: # TODO(katesonia) Add feedback page for Cracas after Cracas integration. 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．
On 2016/11/29 20:49:43, Sharu Jiang wrote: > https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... > File appengine/findit/model/crash/cracas_crash_analysis.py (right): > > https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... > appengine/findit/model/crash/cracas_crash_analysis.py:15: # TODO(katesonia) Add > feedback page for Cracas after Cracas integration. > 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． Another concern is that 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.
lgtm, with nits https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/crash_analysis.py:163: Note, this function must be called by concret subclass of CrashAnalysis "concret" -> "a concrete" https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/crash_analysis.py:164: which implements ProcessResultForPublishing abstract function. -> "which implements the ProcessResultForPublishing method." https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/crash_analysis.py:185: 'client_id': self.client_id, Since you removed the line where we initialized self.client_id (170 in master), it may not be defined here
https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/crash_analysis.py:163: Note, this function must be called by concret subclass of CrashAnalysis On 2016/11/29 22:11:12, wrengr wrote: > "concret" -> "a concrete" Done. https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/crash_analysis.py:164: which implements ProcessResultForPublishing abstract function. On 2016/11/29 22:11:12, wrengr wrote: > -> "which implements the ProcessResultForPublishing method." Done. https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/crash_analysis.py:185: 'client_id': self.client_id, On 2016/11/29 22:11:12, wrengr wrote: > Since you removed the line where we initialized self.client_id (170 in master), > it may not be defined here I think the self.client_id would always exists since it's a property of CrashAnalysis.
lgtm I don't want to block this CL just due to my question on the unimplemented TODO. https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/cracas_crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/cracas_crash_analysis.py:15: # TODO(katesonia) Add feedback page for Cracas after Cracas integration. 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? https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/crash_analysis.py:163: Note, this function must be called by concret subclass of CrashAnalysis On 2016/11/30 02:37:00, Sharu Jiang wrote: > On 2016/11/29 22:11:12, wrengr wrote: > > "concret" -> "a concrete" > > Done. I don't think this is actually done. The misspell still exists.
https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/cracas_crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/cracas_crash_analysis.py:15: # TODO(katesonia) Add feedback page for Cracas after Cracas integration. 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. https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/crash_analysis.py:163: Note, this function must be called by concret subclass of CrashAnalysis On 2016/11/30 04:35:07, stgao (slow on Monday) wrote: > On 2016/11/30 02:37:00, Sharu Jiang wrote: > > On 2016/11/29 22:11:12, wrengr wrote: > > > "concret" -> "a concrete" > > > > Done. > > I don't think this is actually done. The misspell still exists. Oops, done.
On 2016/11/30 18:14:07, Sharu Jiang wrote: > 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 agree it shouldn't be in crash_pipeline.py since that part is agnostic to the client (other than creating an appropriate object for it). FWIW, I don't have a problem putting that in FinditFor*. I had two main goals motivating the Findit classes: (1) to separate all the webapp stuff from the Predator class, (2) to capture the client conditionals once and for all so we don't branch on the client in every single method/function like we used to. The distinction between the Findit classes and the ndb.Model stuff is a lot hazier, so I can see things moving back and forth between them until we figure out the cleanest organization. FWIW, I'm also fine moving the Findit classes to the model directory, or merging the classes together, or whatever else you think is reasonable— provided my two motivating goals are still met. Since (our use of) ndb.Models are just for serializing Python objects, there will always be a bit of a question about which side of the fence to do this sort of processing on. So long as we cleanly separate out the Python objects (which provide the internal API for passing things around inside the program) from the ndb.Models (which provide the external API for serializing things for use on the wire and between processes), where we do the processing itself doesn't really matter (since it's just part of the mapping between the two APIs).
https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/cracas_crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/cracas_crash_analysis.py:15: # TODO(katesonia) Add feedback page for Cracas after Cracas integration. 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?
On 2016/11/30 18:51:56, wrengr wrote: > On 2016/11/30 18:14:07, Sharu Jiang wrote: > > 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 agree it shouldn't be in crash_pipeline.py since that part is agnostic to the > client (other than creating an appropriate object for it). > > FWIW, I don't have a problem putting that in FinditFor*. I had two main goals > motivating the Findit classes: (1) to separate all the webapp stuff from the > Predator class, (2) to capture the client conditionals once and for all so we > don't branch on the client in every single method/function like we used to. The > distinction between the Findit classes and the ndb.Model stuff is a lot hazier, > so I can see things moving back and forth between them until we figure out the > cleanest organization. FWIW, I'm also fine moving the Findit classes to the > model directory, or merging the classes together, or whatever else you think is > reasonable— provided my two motivating goals are still met. Since (our use of) > ndb.Models are just for serializing Python objects, there will always be a bit > of a question about which side of the fence to do this sort of processing on. So > long as we cleanly separate out the Python objects (which provide the internal > API for passing things around inside the program) from the ndb.Models (which > provide the external API for serializing things for use on the wire and between > processes), where we do the processing itself doesn't really matter (since it's > just part of the mapping between the two APIs). It seems to me not a good idea to move the Findit classes to the model directory, because the model directory is set up for code to read/write/update ndb, and simple processing of the data __retrieved from the ndb__. However, the url to the feedback page is not part of the ndb data.
On 2016/11/30 18:51:56, wrengr wrote: > On 2016/11/30 18:14:07, Sharu Jiang wrote: > > 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 agree it shouldn't be in crash_pipeline.py since that part is agnostic to the > client (other than creating an appropriate object for it). > > FWIW, I don't have a problem putting that in FinditFor*. I had two main goals > motivating the Findit classes: (1) to separate all the webapp stuff from the > Predator class, (2) to capture the client conditionals once and for all so we > don't branch on the client in every single method/function like we used to. The > distinction between the Findit classes and the ndb.Model stuff is a lot hazier, > so I can see things moving back and forth between them until we figure out the > cleanest organization. FWIW, I'm also fine moving the Findit classes to the > model directory, or merging the classes together, or whatever else you think is > reasonable— provided my two motivating goals are still met. Since (our use of) > ndb.Models are just for serializing Python objects, there will always be a bit > of a question about which side of the fence to do this sort of processing on. So > long as we cleanly separate out the Python objects (which provide the internal > API for passing things around inside the program) from the ndb.Models (which > provide the external API for serializing things for use on the wire and between > processes), where we do the processing itself doesn't really matter (since it's > just part of the mapping between the two APIs). It seems putting all the ToPublishableResult to FinditFor* is the good way to go.
https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... File appengine/findit/model/crash/cracas_crash_analysis.py (right): https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... appengine/findit/model/crash/cracas_crash_analysis.py:15: # TODO(katesonia) Add feedback page for Cracas after Cracas integration. 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.
On 2016/11/30 19:26:05, Sharu Jiang wrote: > https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... > File appengine/findit/model/crash/cracas_crash_analysis.py (right): > > https://codereview.chromium.org/2523343002/diff/40001/appengine/findit/model/... > appengine/findit/model/crash/cracas_crash_analysis.py:15: # TODO(katesonia) Add > feedback page for Cracas after Cracas integration. > 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. I'm fine with that.
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
lgtm
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2523343002/#ps200001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1480547671446400,
"parent_rev": "52efea5d0439ec8bcab5000a5ee4dfbc70660bb0", "commit_rev":
"d10052ed79ae195e1569f18f52d24990b35f3a4f"}
Message was sent while issue was closed.
Description was changed from ========== [Predator] Refactor ToPublishResult and fix keyerror 'found' BUG=668303 ========== to ========== [Predator] Refactor ToPublishResult and fix keyerror 'found' BUG=668303 Committed: https://chromium.googlesource.com/infra/infra/+/d10052ed79ae195e1569f18f52d24... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001) as https://chromium.googlesource.com/infra/infra/+/d10052ed79ae195e1569f18f52d24... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
