Improve queryFakeItems to return more data, including different types of list items

RESOLVED FIXED in Firefox 29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 29
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: shovel-ready)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
To make it easy to develop the UI bits of the list work, we should create a way to get a Cursor with a bunch of test data.
(Assignee)

Comment 1

5 years ago
Bug 941318 took care of adding a "fake" data endpoint, but we should flesh it out more to return different kinds of data.

We also discussed pulling this data from a JSON file, perhaps letting a test add-on specify which file to use to get that data.
No longer blocks: 941318
Depends on: 941318
Summary: Create an endpoint for list content provider that returns static test data → Improve queryFakeItems to return more data, including different types of list items
(Assignee)

Updated

5 years ago
Assignee: margaret.leibovic → nobody
Whiteboard: shovel-ready
(Assignee)

Updated

5 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 2

5 years ago
Created attachment 8357508 [details] [diff] [review]
(Part 1) Rename PROVIDER_ID to LIST_ID

This renames the PROVIDER_ID column to LIST_ID, to make it more clear that we can have more than one list per provider.
Attachment #8357508 - Flags: review?(wjohnston)
(Assignee)

Comment 3

5 years ago
Created attachment 8357510 [details] [diff] [review]
(Part 2) Pull fake list items from a JSON file

This patch lets us pull fake items from a JSON file. I wrapped this extra file in nightly build defines, since we wouldn't want to ship this file.

In a future patch we can add more data to this file, but this will also make it easier for developers to add their own things to this file (although they'd still need to update the Java if they want more columns).

Also, I modeled this after recommended-addons.json, but I filed bug 957881 to remove that, since I noticed I don't think we use that file anymore.

And I still need to write to let us actually filter these list items per list id.
Attachment #8357510 - Flags: review?(wjohnston)
(Assignee)

Comment 4

5 years ago
(In reply to :Margaret Leibovic from comment #2)
> Created attachment 8357508 [details] [diff] [review]
> (Part 1) Rename PROVIDER_ID to LIST_ID
> 
> This renames the PROVIDER_ID column to LIST_ID, to make it more clear that
> we can have more than one list per provider.

(In my mind, "provider" means a data provider that chooses to ship an add-on, such as a news provider shipping an add-on with multiple lists for different news sections.)
(Assignee)

Comment 5

5 years ago
Comment on attachment 8357510 [details] [diff] [review]
(Part 2) Pull fake list items from a JSON file

I just realized that this will cause testHomeListsProvider to fail when it hits aurora... maybe I should just get rid of that test for fake items. Or maybe I can put that behind a build flag as well.
(Assignee)

Comment 6

5 years ago
Comment on attachment 8357508 [details] [diff] [review]
(Part 1) Rename PROVIDER_ID to LIST_ID

Don't want this patch anymore.
Attachment #8357508 - Attachment is obsolete: true
Attachment #8357508 - Flags: review?(wjohnston)
(Assignee)

Comment 7

5 years ago
Comment on attachment 8357510 [details] [diff] [review]
(Part 2) Pull fake list items from a JSON file

Downgrade to feedback... I want to update this based on the discussion in our meeting this morning.
Attachment #8357510 - Flags: review?(wjohnston) → feedback?(wjohnston)
(Assignee)

Comment 8

5 years ago
Created attachment 8360790 [details] [diff] [review]
Pull fake list items from a JSON file (without renaming things)

Given that we've still been discussing how we'll do the real backend storage here, let's punt on renaming things just yet, but I'd like to land this patch, since it will make it easier for developers to hack on the UI views without waiting for the backend bits to work.

To test this, I just threw this line of code inside HomeConfigPrefsBackend.loadDefaultConfig():

panelConfigs.add(new PanelConfig(PanelType.DYNAMIC, "Dynamic", dynamic"));

This works because right now DynamicPanel just creates a ListView that's backed by a Cursor returned from queryFakeItems. This will change as lucasr works in bug 959777 to make DynamicPanel actually do what it's supposed to do, but even if that code lands, a developer could easily plug in whatever views they want to try to test (ideally, the could just create them with a PanelConfig).
Attachment #8357510 - Attachment is obsolete: true
Attachment #8357510 - Flags: feedback?(wjohnston)
Attachment #8360790 - Flags: review?(wjohnston)
(Assignee)

Comment 9

5 years ago
As part of this patch, I also think we should just disable testHomeListsProvider. It's not actually doing much of anything useful right now, so I don't want to invest in updating it until we change HomeListsProvider around to be more useful (bug 961092).
Comment on attachment 8360790 [details] [diff] [review]
Pull fake list items from a JSON file (without renaming things)

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

Nice. I think you can simplify it a little actually though :) yay!?

::: mobile/android/base/db/HomeListsProvider.java
@@ +415,5 @@
> +    private String getRawFakeItems() throws IOException {
> +        final ZipFile zipFile = new ZipFile(getContext().getPackageResourcePath());
> +        final ZipEntry zipEntry = zipFile.getEntry("fake-list-items.json");
> +
> +        final InputStream inputStream = zipFile.getInputStream(zipEntry);

It should be simpler to just ship this as a raw resource. i.e. add a raw folder in mobile/android/base/resources. I think our build will just move it for you now (thanks nalexander!), and then use:

InputStream inputStream = getResources().openRawResource(R.raw.fake_list_items);
Attachment #8360790 - Flags: review?(wjohnston) → review-
(Assignee)

Comment 11

5 years ago
Created attachment 8364037 [details] [diff] [review]
Pull fake list items from a JSON file (v3)

Nice suggestion :)

I think the robocop test was failing before just because I renamed something without updating the test. It passes locally for me now, and I pushed to try to verify:
https://tbpl.mozilla.org/?tree=Try&rev=371d82f82324

As much as this test isn't useful in that it isn't testing code we ship, it is useful to test this content provider endpoint, and to make sure it doesn't get broken.
Attachment #8360790 - Attachment is obsolete: true
Attachment #8364037 - Flags: review?(wjohnston)
(Assignee)

Updated

5 years ago
Blocks: 952310
Attachment #8364037 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/a1f08c3b158a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.