You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Doug Davies <da...@oclc.org> on 2015/05/26 20:26:43 UTC
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/
-----------------------------------------------------------
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
Posted by Ryan Baxter <rb...@gmail.com>.
> 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
>
> Doug Davies wrote:
> Hmmmmm... 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)?
You mean do we have an existing test?
- 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
Posted by Doug Davies <da...@oclc.org>.
> 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
Posted by Ryan Baxter <rb...@gmail.com>.
> 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
Posted by Doug Davies <da...@oclc.org>.
> 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
Hmmmmm... 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
Posted by Doug Davies <da...@oclc.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34673/#review85425
-----------------------------------------------------------
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).
- Doug Davies
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
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
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
Posted by Doug Davies <da...@oclc.org>.
-----------------------------------------------------------
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 (updated)
-------
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
Posted by Doug Davies <da...@oclc.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34673/
-----------------------------------------------------------
(Updated June 15, 2015, 8:04 p.m.)
Review request for shindig.
Changes
-------
Added jsunit test for re-navigating when moduleId is set (wow, how specific is that?)
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
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.
Thanks,
Doug Davies
Re: Review Request 34673: GadgetSite setModuleId_ only sets the
module id if rendering the first time
Posted by Doug Davies <da...@oclc.org>.
-----------------------------------------------------------
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
Posted by Doug Davies <da...@oclc.org>.
> 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.
>
> Ryan Baxter wrote:
> was actually talking about this if statement
>
> ```
> if (mid || mid == 0) {
> self.moduleId_ = mid;
> }
> ```
Possibly. I was figuring the outer if statement (mid && this.moduleId_ != mid) was taking care of this. But I can check again if desired.
- 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
Posted by Doug Davies <da...@oclc.org>.
> 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
Posted by Ryan Baxter <rb...@gmail.com>.
> 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
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
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
>
>