Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/#review88072 --- Ship it! Ship It! - Ryan Baxter On June 16, 2015, 1:32 p.m., Doug Davies wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/ --- (Updated June 16, 2015, 1:32 p.m.) Review request for shindig. Bugs: SHINDIG-1995 https://issues.apache.org/jira/browse/SHINDIG-1995 Repository: shindig Description --- GadgetSite setModuleId_ only sets the module id if rendering the first time Diffs - trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js 1681771 trunk/features/src/test/javascript/features/container/gadget_site_test.js 1681771 Diff: https://reviews.apache.org/r/34673/diff/ Testing --- See bug (SHINDIG-1995) for details on how to test this. UPDATE: there is now a testReNavigateToWithModuleId unit test in gadget_site_test.js. Thanks, Doug Davies
Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time
On May 27, 2015, 7:43 p.m., Doug Davies wrote: Added another patch that tries to re-use the previous moduleId off the cached token. It also checks if mid is defined (as suggested by Ryan). Ryan Baxter wrote: Looks good Doug, is there any kind of unit tests we can add around this? I'll see if I can add a js test - Doug --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/#review85425 --- On May 27, 2015, 7:39 p.m., Doug Davies wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/ --- (Updated May 27, 2015, 7:39 p.m.) Review request for shindig. Bugs: SHINDIG-1995 https://issues.apache.org/jira/browse/SHINDIG-1995 Repository: shindig Description --- GadgetSite setModuleId_ only sets the module id if rendering the first time Diffs - trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js 1681771 Diff: https://reviews.apache.org/r/34673/diff/ Testing --- See bug (SHINDIG-1995) for details on how to test this. Thanks, Doug Davies
Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time
On May 27, 2015, 7:43 p.m., Doug Davies wrote: Added another patch that tries to re-use the previous moduleId off the cached token. It also checks if mid is defined (as suggested by Ryan). Ryan Baxter wrote: Looks good Doug, is there any kind of unit tests we can add around this? Doug Davies wrote: I'll see if I can add a js test Hm... js tests are hard. :) I played with this most of today and was unsuccessful. Do we have to have a test for this change to be accepted (other than the sample I showed)? - Doug --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/#review85425 --- On May 27, 2015, 7:39 p.m., Doug Davies wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/ --- (Updated May 27, 2015, 7:39 p.m.) Review request for shindig. Bugs: SHINDIG-1995 https://issues.apache.org/jira/browse/SHINDIG-1995 Repository: shindig Description --- GadgetSite setModuleId_ only sets the module id if rendering the first time Diffs - trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js 1681771 Diff: https://reviews.apache.org/r/34673/diff/ Testing --- See bug (SHINDIG-1995) for details on how to test this. Thanks, Doug Davies
Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time
On May 27, 2015, 4:21 a.m., Ryan Baxter wrote:
trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js,
line 128
https://reviews.apache.org/r/34673/diff/1/?file=971865#file971865line128
Should this be surrounded in the same if statement as above?
Doug Davies wrote:
No, that's the problem. !self.service_.getCachedGadgetToken(url) is
FALSE (token exists because we are re-rendering). In that case we drop
through, but the moduleId is never set.
was actually talking about this if statement
```
if (mid || mid == 0) {
self.moduleId_ = mid;
}
```
- Ryan
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34673/#review85315
---
On May 26, 2015, 6:26 p.m., Doug Davies wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34673/
---
(Updated May 26, 2015, 6:26 p.m.)
Review request for shindig.
Bugs: SHINDIG-1995
https://issues.apache.org/jira/browse/SHINDIG-1995
Repository: shindig
Description
---
GadgetSite setModuleId_ only sets the module id if rendering the first time
Diffs
-
trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js
1681771
Diff: https://reviews.apache.org/r/34673/diff/
Testing
---
See bug (SHINDIG-1995) for details on how to test this.
Thanks,
Doug Davies
Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time
On May 27, 2015, 4:21 a.m., Ryan Baxter wrote: trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js, line 128 https://reviews.apache.org/r/34673/diff/1/?file=971865#file971865line128 Should this be surrounded in the same if statement as above? No, that's the problem. !self.service_.getCachedGadgetToken(url) is FALSE (token exists because we are re-rendering). In that case we drop through, but the moduleId is never set. - Doug --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/#review85315 --- On May 26, 2015, 6:26 p.m., Doug Davies wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/ --- (Updated May 26, 2015, 6:26 p.m.) Review request for shindig. Bugs: SHINDIG-1995 https://issues.apache.org/jira/browse/SHINDIG-1995 Repository: shindig Description --- GadgetSite setModuleId_ only sets the module id if rendering the first time Diffs - trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js 1681771 Diff: https://reviews.apache.org/r/34673/diff/ Testing --- See bug (SHINDIG-1995) for details on how to test this. Thanks, Doug Davies
Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/ --- (Updated May 27, 2015, 7:39 p.m.) Review request for shindig. Bugs: SHINDIG-1995 https://issues.apache.org/jira/browse/SHINDIG-1995 Repository: shindig Description --- GadgetSite setModuleId_ only sets the module id if rendering the first time Diffs (updated) - trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js 1681771 Diff: https://reviews.apache.org/r/34673/diff/ Testing --- See bug (SHINDIG-1995) for details on how to test this. Thanks, Doug Davies
Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time
On May 27, 2015, 7:43 p.m., Doug Davies wrote: Added another patch that tries to re-use the previous moduleId off the cached token. It also checks if mid is defined (as suggested by Ryan). Looks good Doug, is there any kind of unit tests we can add around this? - Ryan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/#review85425 --- On May 27, 2015, 7:39 p.m., Doug Davies wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/ --- (Updated May 27, 2015, 7:39 p.m.) Review request for shindig. Bugs: SHINDIG-1995 https://issues.apache.org/jira/browse/SHINDIG-1995 Repository: shindig Description --- GadgetSite setModuleId_ only sets the module id if rendering the first time Diffs - trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js 1681771 Diff: https://reviews.apache.org/r/34673/diff/ Testing --- See bug (SHINDIG-1995) for details on how to test this. Thanks, Doug Davies
Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/#review85315 --- trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js https://reviews.apache.org/r/34673/#comment136872 Should this be surrounded in the same if statement as above? - Ryan Baxter On May 26, 2015, 6:26 p.m., Doug Davies wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34673/ --- (Updated May 26, 2015, 6:26 p.m.) Review request for shindig. Bugs: SHINDIG-1995 https://issues.apache.org/jira/browse/SHINDIG-1995 Repository: shindig Description --- GadgetSite setModuleId_ only sets the module id if rendering the first time Diffs - trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js 1681771 Diff: https://reviews.apache.org/r/34673/diff/ Testing --- See bug (SHINDIG-1995) for details on how to test this. Thanks, Doug Davies
