"submitted at" field doesn't always display on ship it's completed releases (and maybe elsewhere)

RESOLVED FIXED

Status

Release Engineering
Release Automation: Other
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhearsum, Assigned: massimo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [shipit])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Massimo, you pointed this out to me in dev, but I thought it was just because of the data in dev. Turns out this is an issue in production too...so there's something mucked up somewhere.
(Assignee)

Comment 1

5 years ago
Created attachment 728977 [details] [diff] [review]
added a function to update submitted_at when table changes

I think the issue is caused by some lazy load mechanism in dataTables library. Elements on the first page are fine, rows in the next pages are empty or show the wrong data. 

This patch adds a toLocalDate() function that is called when the page is loaded and when the table changes. 

I've tested this patch locally and it works fine. Is it possible to have a production database dump to check how it behaves with real data?
Attachment #728977 - Flags: review?(bhearsum)
(Reporter)

Comment 2

5 years ago
Comment on attachment 728977 [details] [diff] [review]
added a function to update submitted_at when table changes

Review of attachment 728977 [details] [diff] [review]:
-----------------------------------------------------------------

This patch doesn't work for me...I end up with "invalid date" for all of the releases. I tried to debug a bit locally and it looks toLocaleDate is called once at page load, and then once right afterwards (probably on the fwDrawCallback event). I think what's happening is it's getting set correctly on the first call, and then it tries to read in the already localized date on the second call and doesn't know what to do with it.

::: kickoff/templates/includes/releases_reviewed.html
@@ +64,2 @@
>  <script type='text/javascript'>
> +function toLocalDate() {

This is getting defined twice...can you put it in a central place? Maybe it's time to have a kickoff.js for things like this?
Attachment #728977 - Flags: review?(bhearsum) → review-
(Reporter)

Comment 3

5 years ago
Comment on attachment 728977 [details] [diff] [review]
added a function to update submitted_at when table changes

Review of attachment 728977 [details] [diff] [review]:
-----------------------------------------------------------------

We did a bit more debugging on this. My hunch about things being called multiple times was right -- toLocalDate gets called twice on load (once in releases_complete, once in releases_reviewed). The second time it gets called, its input is an already localized date. Depending on your system language format, this may or may not work (it worked for Massimo, didn't work for me). I don't think fnDrawCallback is necessary here - in my local fiddling it only gets called once on page load, which makes it effectively the same as running something through $(document).ready(). I think the only thing needed is a _single_ call to toLocalDate(). This should probably be '$(document).ready(toLocalDate)' in releases.html.

::: kickoff/templates/submit_release.html
@@ -24,5 @@
>     {% endfor %}
>  </div>
> -<script>
> - $(document).ready(function() {
> -   $( "#submitrelease" ).prop('checked', true);

Also, what's this change about? It seems unrelated.
(Assignee)

Comment 4

5 years ago
Created attachment 732795 [details] [diff] [review]
fix for empty submitted_at columns

I had problems calling the local date function on document ready so I had to split the toLocalDate in two different functions: one for complete/reviewed releases and one for submitted releases. 

I've also moved javascript code into kickoff.js as suggested in comment #2
Attachment #728977 - Attachment is obsolete: true
Attachment #732795 - Flags: review?(bhearsum)
(Reporter)

Comment 5

5 years ago
Comment on attachment 732795 [details] [diff] [review]
fix for empty submitted_at columns

Review of attachment 732795 [details] [diff] [review]:
-----------------------------------------------------------------

::: kickoff/static/kickoff.js
@@ +1,3 @@
> +function initialSetup(){
> +  $( "#tabs" ).tabs();
> +  $( "#accordion" ).accordion({ heightStyle: "content" });

I see aaSorting disappearing...is that intended? Do you need to merge/rebase?

@@ +5,5 @@
> +
> +function onTableChange(release_type) {
> +    // entry point for dataTable events
> +    toLocalDate(release_type);
> +}

What's the point of this function? Can't you use toLocalDate as the callback directly?

::: kickoff/templates/releases.html
@@ +31,5 @@
> +<script>
> +    $(document).ready(function(){
> +        $( "#reviewed" ).dataTable({
> +            "bJQueryUI": true,
> +            "aaSorting": [[ 2, "desc" ]], // sort by SubittedAt

typo nit: SubittedAt vs. SubmittedAt.

@@ +32,5 @@
> +    $(document).ready(function(){
> +        $( "#reviewed" ).dataTable({
> +            "bJQueryUI": true,
> +            "aaSorting": [[ 2, "desc" ]], // sort by SubittedAt
> +            "fnDrawCallback": onTableChange('reviewed'),

Are you sure this works? It looks like you're using the return value of onTableChange() as the callback. I think this is effectively the same as calling onTableChange in document.ready()....

If you're calling this here you need to wrap it in a function(){}.

@@ +39,5 @@
> +            "bJQueryUI": true,
> +            "aaSorting": [[ 2, "desc" ]], // sort by SubittedAt
> +            "fnDrawCallback": onTableChange('complete'),
> +        });
> +    updateSubmitLocalDate();

You're effectively doing the same thing twice again....

I tried this locally:
<script>
    $(document).ready(function(){
        $( "#reviewed" ).dataTable({
            "bJQueryUI": true,
            "aaSorting": [[ 2, "desc" ]], // sort by SubittedAt
            "fnDrawCallback": toLocalDate('reviewed'),
        });
        $( "#complete" ).dataTable({
            "bJQueryUI": true,
            "aaSorting": [[ 2, "desc" ]], // sort by SubittedAt
            "fnDrawCallback": toLocalDate('complete'),
        });
});


and deleted the onTableChange and updateSubmitLocalDate functions, and everything still worked fine. I think this code is still overcomplicating the issue.
Attachment #732795 - Flags: review?(bhearsum) → review-
(Assignee)

Comment 6

5 years ago
Created attachment 733301 [details] [diff] [review]
fix for empty submitted_at columns

Hi Ben,

I've done another test running toLocalDate() when the page is ready. It works fine only if the tables have a single page, from the second page, dates are just the same string provided by the backend.

The attached patch resolves the issue updating 'submitted at' fields before initializing the datatables, so when table is loaded, dates are already in the correct format and there is no need for fnDrawCallback.
Attachment #732795 - Attachment is obsolete: true
Attachment #733301 - Flags: review?(bhearsum)
(Reporter)

Updated

5 years ago
Attachment #733301 - Flags: review?(bhearsum)
Attachment #733301 - Flags: review+
Attachment #733301 - Flags: checked-in+
(Reporter)

Comment 7

5 years ago
I pushed this to ship it prod.
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.