history field view doesn't work on integer columns

RESOLVED FIXED

Status

Release Engineering
Applications: Balrog (backend)
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bhearsum, Assigned: ninad101, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=python][good first bug])

Attachments

(3 obsolete attachments)

(Reporter)

Description

2 years ago
https://github.com/mozilla/balrog/blob/cc7405f4d47f369b9f1c60380ae142432deb275c/auslib/admin/views/history.py#L31 ultimately tries to coerce everything to unicode....which fails on int. Eg:
self = <auslib.admin.views.history.FieldView object at 0x7f23638f5bd0>
value = 2000

    def format_value(self, value):
        if isinstance(value, basestring):
            try:
                value = json.loads(value)
                value = json.dumps(value, indent=2, sort_keys=True)
            except ValueError:
                pass
        elif value is None:
            value = 'NULL'
        else:
>           value = unicode(value, 'utf8')
E           TypeError: coercing to Unicode: need string or buffer, int found

auslib/admin/views/history.py:42: TypeError
(Reporter)

Updated

2 years ago
Mentor: bhearsum
(Reporter)

Updated

2 years ago
Whiteboard: [lang=python][good first bug]
(Assignee)

Comment 1

2 years ago
Created attachment 8790428 [details] [diff] [review]
Adds extra check for integer

I will like to work on this bug. Will an extra check for integer be enough?
Flags: needinfo?(bhearsum)
(Assignee)

Comment 2

2 years ago
Created attachment 8790430 [details] [diff] [review]
Test_Bug_1278543.patch
Attachment #8790428 - Attachment is obsolete: true
(Reporter)

Comment 3

2 years ago
This might do it, but the best way to find out is to write a test to make sure it fails without your patch. This will help make sure it doesn't regress in the future, too! There's a few existing history tests in https://github.com/mozilla/balrog/blob/master/auslib/test/admin/views/test_history.py, you'll want to write one that modifies one of the numeric values on a rule, and then tries retrieving that field from an endpoint like /history/view/rules/1/priority.

The existing history tests have a database that's get populated by https://github.com/mozilla/balrog/blob/master/auslib/test/admin/views/base.py#L14, so there should be a few rules to work with already. I know this might be a bit overwhelming when you're not familiar with the code, so just let me know if you need a hand creating the test.

You can send the next iteration as a pull request to https://github.com/mozilla/balrog as well.
Flags: needinfo?(bhearsum)
(Reporter)

Updated

2 years ago
Assignee: nobody → bhat.ninadmb
(Assignee)

Comment 4

2 years ago
Created attachment 8790924 [details] [diff] [review]
Final patch

should I make changes in 
https://github.com/mozilla/balrog/blob/master/auslib/admin/views/base.py#L106 
too
Flags: needinfo?(bhearsum)
(Reporter)

Comment 5

2 years ago
(In reply to Ninad Bhat[:ninad101] from comment #4)
> Created attachment 8790924 [details] [diff] [review]
> Final patch
> 
> should I make changes in 
> https://github.com/mozilla/balrog/blob/master/auslib/admin/views/base.
> py#L106 
> too

We discussed this on IRC, work has now moved to https://github.com/mozilla/balrog/pull/124.
Flags: needinfo?(bhearsum)
(Reporter)

Updated

2 years ago
Attachment #8790430 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8790924 - Attachment is obsolete: true

Comment 6

2 years ago
Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/9fdf16f9c139e39c45a7e0e741eb0b6a741be17d
Bug 1278543: history field view doesn't work on integer columns (#124). r=bhearsum
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INVALID
(Reporter)

Comment 7

2 years ago
Fixing the resolution, this is FIXED, not INVALID :). It'll get deployed to production next week.
Resolution: INVALID → FIXED
(Reporter)

Updated

2 years ago
Depends on: 1304054
You need to log in before you can comment on or make changes to this bug.