published needs to be enforced

RESOLVED DUPLICATE of bug 922166

Status

RESOLVED DUPLICATE of bug 922166
5 years ago
5 years ago

People

(Reporter: thecount, Assigned: mjschranz)

Tracking

Details

(Whiteboard: preworkweek)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
I'm working on the front end of allowing projects to be saved privatly, then later published in bug 913731.

In order for this to mean anything, we need it to be enforced from inside the MakeAPI.

We're going to limit the public search to never return isPublished: false just like it doesn't return deleted makes.

Then we need an authenticated search. I'm thinking the authenticated search can be a new ticket.
(Reporter)

Updated

5 years ago
Assignee: nobody → scott
(Reporter)

Updated

5 years ago
Blocks: 913731
(Reporter)

Comment 1

5 years ago
Created attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

Here it is.

No review request yet, as I'm still testing it.

It's not easy to test, so I'm basically going to implement the other side in popcorn maker. Once both patches are working together, I'll put this in review.

The current solution to this is expose an authenticatedSearch for apps to call to get un published and deleted makes.
(Reporter)

Updated

5 years ago
Whiteboard: mozfest
(Reporter)

Comment 2

5 years ago
Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

Probably easiest to test with bug 913731 as that allows you to publish a make to the makeapi with published === false, by hitting just save, then try to load that make again and it should be filtered out.

This one blocks bug 913731 though so obviously bug 913731 cannot land for multiple reasons.
Attachment #809464 - Flags: review?(cade)
(Reporter)

Comment 3

5 years ago
Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

Adding a second reviewer because Chris is fairly busy.
Attachment #809464 - Flags: review?(schranz.m)
(Assignee)

Comment 4

5 years ago
Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

Logic wise, this appears to be correct. I was able get some testing by saving projects in your save/publish PM patch before using this patch (saw them in a search for my username) and then with it (they were not present in a search for my username).

Minor nits, but I'll let Chris get back to this.
Attachment #809464 - Flags: review?(schranz.m) → review-
Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

Requests that are made with Hawk must also have their responses signed with hawk so that the requester can verify the responses' authenticity and integrity.

Should you write tests for the changes you've made? (custom filters passed into the build function)
Attachment #809464 - Flags: review?(cade) → review-
(Reporter)

Comment 6

5 years ago
Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

I moved authenticated search out, going to do that in another ticket while I figure out what hawk is actually doing.

Keeping this ticket limited to just stripping published via a filter.

We already have tests for the filter we support. I would say write tests for what we support. Once we've added a new filter, we should create a test for it. It's not exposed to anyone on the other end of the call, just internal.

I wouldn't mind a test for making sure a non published project doesn't get returned.
Attachment #809464 - Flags: review- → review?(cade)
Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

r+ with one comment for you to consider in the Pull Request.
Attachment #809464 - Flags: review?(cade) → review+
(Reporter)

Comment 8

5 years ago
Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

Agreed.
Attachment #809464 - Flags: review+ → review?(cade)
(Reporter)

Comment 10

5 years ago
Staged: https://github.com/mozilla/MakeAPI/commit/e8207c8273f6cd64475bc9f9bf7f329f3fa8606e

Needs verification.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(scott)
Resolution: --- → FIXED
(Reporter)

Comment 11

5 years ago
This got backed out because it didn't work on staging.

Needs some kind of migration script to fix the published property.
Status: RESOLVED → REOPENED
Flags: needinfo?(scott)
Resolution: FIXED → ---
(Reporter)

Updated

5 years ago
Summary: isPublished needs to be enforced → published needs to be enforced
moving from mozfest milestone
Whiteboard: mozfest
Attachment mime type: text/plain → text/x-github-pull-request
(Assignee)

Updated

5 years ago
Assignee: scott → schranz.m
Status: REOPENED → ASSIGNED
(Assignee)

Comment 13

5 years ago
At this point there doesn't seem to be any "clean" option to fixing this and we might need to do something by hand.

I believe I can change the mapping manually and it would work but I can't confirm since I can't reproduce the problem locally. The other solution is to basically going to be create a copy of the current data, make new mappings with the correct data and run a script to update all records because old projects have published set to false.

Here's a gist of all the mapping differences I have pulled locally https://gist.github.com/mjschranz/910b0023ef11b833caa0

Jon, I need your help to get this done. Is there a time we can set aside to figure this out and do it?
Flags: needinfo?(jon)
How can you change the mapping manually? Is there a command line script or app you can run to change the indices?
Flags: needinfo?(jon)
(Assignee)

Comment 15

5 years ago
Basically they have this PUT mapping API. Their examples of it use a CURL request and I tried it locally just playing around with my existing mapping and it seemed okay.

Chris seems to think this on it's own will not work but I have no way of knowing for sure since the problem can't be replicated locally.
Flags: needinfo?(jon)

Updated

5 years ago
Blocks: 912696
Okay, after doing a bunch of research and reading http://www.elasticsearch.org/blog/changing-mapping-with-zero-downtime/ I think we should just add a new mapping that isn't named 'published' and forget that 'published' ever existed.

My reasoning here is:
1) You can't delete a mapping(!)
2) We don't care about the data currently stored in the 'published' field, it's currently true for all makes
3) We can't upgrade to a multi-field, because published is currently an "object" type
4) Doing schema migration using aliases as described in the blog post looks amazing, but I think this will require much more setup and planning. Might be worth looking at though!
5) Re-indexing the ES data from scratch in MongoDB will require downtime. Not sure how long, would need to test this.
Flags: needinfo?(jon)
(Assignee)

Comment 18

5 years ago
Created attachment 8340439 [details] [review]
https://github.com/mozilla/makeapi-client/pull/18

Client change.
Attachment #809464 - Attachment is obsolete: true
Attachment #8340439 - Flags: review?(cade)
(Assignee)

Comment 19

5 years ago
Created attachment 8340442 [details] [review]
https://github.com/mozilla/MakeAPI/pull/179

Probably will want to update the client version when that lands in this patch as well.
Attachment #8340442 - Flags: review?(jon)
Attachment #8340442 - Flags: review?(cade)
(Assignee)

Comment 21

5 years ago
Created attachment 8340447 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/74

Goggles Patch.
Attachment #8340447 - Flags: review?(pomax)
Attachment #8340447 - Flags: review?(cade)
(Assignee)

Comment 23

5 years ago
Comment on attachment 8340442 [details] [review]
https://github.com/mozilla/MakeAPI/pull/179

I derped and forgot I need a migration script.
Attachment #8340442 - Flags: review?(jon)
Attachment #8340442 - Flags: review?(cade)
(Assignee)

Comment 24

5 years ago
Comment on attachment 8340442 [details] [review]
https://github.com/mozilla/MakeAPI/pull/179

Added the script!
Attachment #8340442 - Flags: review?(jon)
Attachment #8340442 - Flags: review?(cade)

Comment 25

5 years ago
I used the "published" flag in my https://bugzilla.mozilla.org/show_bug.cgi?id=912696 to separate save and publish for thimble projects. I've read this entire thread, but somewhere between comment 12 and comment 13 someone concluded that published cannot be used, without explaining why. I'm using it quite successfully in my Thimble patch, so I'd very much like to know why after comment 12 it suddenly became a problem.
Comment on attachment 8340442 [details] [review]
https://github.com/mozilla/MakeAPI/pull/179

Lets remove the changes to querybuilder and the tests. We'll not implement any logic that depends on the new field until we have UI for it (needs bug) and an authenticated search method for the my makes page (so that a user can manage their unlisted makes; Needs a bug)
Attachment #8340442 - Flags: review?(cade) → review-

Updated

5 years ago
Attachment #8340449 - Flags: review?(cade) → review+

Updated

5 years ago
Attachment #8340447 - Flags: review?(cade) → review+

Comment 27

5 years ago
The missing rational: the published flag is supposed to be boolean, but is the wrong type on staging and production, without ES supporting an easy way to change the type. Using a new flag is a hack around this, I would like to recommend we instead fix the mistake we made when we deployed ES on staging and production and not tack on new flags. We're going to be maintaining this for (hopefully) a long time, polluting the schema like this is wilfully turning a blind eye early in the maintenance process. I'd much rather see us come up with a way to fix this ES issue on staging/production first, so that we have an established way to deal with it when we run into it in the future again. Which we almost certainly will. Let's do this right, and then once that's fixed, use the "published" flag in the patches.
Flags: needinfo?(david.humphrey)

Updated

5 years ago
Attachment #8340444 - Flags: review?(cade) → review+

Updated

5 years ago
Attachment #8340439 - Flags: review?(cade) → review+

Updated

5 years ago
Attachment #8340442 - Flags: review?(jon)

Updated

5 years ago
Depends on: 944816

Comment 28

5 years ago
Comment on attachment 8340447 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/74

r- until it can be proven that we need to go down this road, rather than fixing the actual problem of ES having the wrong schema on staging and production.
Attachment #8340447 - Flags: review?(pomax) → review-

Comment 29

5 years ago
Comment on attachment 8340449 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/306

r- until it can be proven that we need to go down this road, rather than fixing the actual problem of ES having the wrong schema on staging and production.
Attachment #8340449 - Flags: review?(pomax) → review-
(Assignee)

Comment 30

5 years ago
I'm still in the camp that doing these potential solutions is not worth it when we can simply add the new flag and removed the old unused field.

In anycase, the solutions Pomax want's are against things I do not have access to so I can't test this out myself. Putting on you Jon.
Assignee: schranz.m → jon
(Reporter)

Comment 31

5 years ago
I'm in the camp of until there is some sort of rush on this, we should look into doing it right.

Am I missing how the above patch removes the olf unused flag? I think the only problem here is the old flag is left to rot while we tack on a new one. Changing the name is kinda not important. If we can remove the old, and add a new, we have one way to do this right.
(Assignee)

Comment 32

5 years ago
Your right in that my patch doesn't actually remove that old flag. I think that would by and large accomplish what we need/what right now. I also think that when we get around to dropping Mongoosastic/Mongo this is a solution that can be handled then (or truthfully we could just manually delete the mappings).

I really would prefer to go with that solution in all honesty. Means we can get the work done faster and no downtime.
Assignee: jon → schranz.m
Flags: needinfo?(pomax)
Both this and bug 944816 seem to be about issues with our ES deployments/indices, so I'd like to see us get a bug (maybe we have one already?) on dealing with those problems directly vs solving them in our apps or the MakeAPI.
(Assignee)

Updated

5 years ago
Assignee: schranz.m → jon

Comment 34

5 years ago
I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=945865 for us so we can figure out how to do elasticsearch migrations. I'd like to see us tackle that bug before we continue work on this bug.
Flags: needinfo?(pomax)

Updated

5 years ago
Duplicate of this bug: 942099

Updated

5 years ago
Whiteboard: preworkweek
Assigning back to Matt because he did the work behind the patches. I still need to fix bug 944816 though, if only to add an alias to instead of directly accessing the index.
Assignee: jon → schranz.m
Flags: needinfo?(david.humphrey)
(Assignee)

Comment 37

5 years ago
Correct me if I'm wrong but we still need to wait on bug 944816? If so let's mark that as a blocker.
Yep, it's set as a blocker
(Reporter)

Comment 39

5 years ago
So what's the plan on this once bug 944816 lands?

Is my initial backed out patch still valid?

https://github.com/mozilla/MakeAPI/commit/e8207c8273f6cd64475bc9f9bf7f329f3fa8606e

All that did was ensure makes that are private do not get returned from client side, and because right now every make is set as not private, everything should still be searchable.

bug 922166 makes it so server side can search for private makes. Then like bug 913731 every app is going to need some sort of UI or authenticated search patch.

Is this still the plan?
Yep, that sounds correct.
(Assignee)

Updated

5 years ago
Attachment #8340439 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8340442 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8340444 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8340447 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8340449 - Attachment is obsolete: true
(Assignee)

Comment 42

5 years ago
So at this point I'm assuming we want patches for this bug to basically be:

- Ensuring normal searches don't return "private" makes
- Fix up all apps to set published false on initial create, and true on their respective publish steps.
- Also change the default of published to true.
- Ensure MakeAPI client doesn't need changes too.

Sound right?
Flags: needinfo?(jon)
Yeah, there will be some sort of authenticated search path that will show unlisted makes, which is being done in bug 922166. I'm not certain how the published flag should be handled, I guess that depends on how each app handles that saved vs published distinction? We might want to default to published = true too.
Flags: needinfo?(jon)
(Assignee)

Comment 44

5 years ago
Actually it's already set to default to true and that seems to be where we want it for now. This patch basically seems to be just now the first bullet point.
Comment on attachment 8366178 [details] [review]
https://github.com/mozilla/MakeAPI/pull/189

There's some tests that need to be fixed up too
Attachment #8366178 - Flags: review?(jon)
Attachment #8366178 - Flags: review?(cade)
Attachment #8366178 - Flags: review-
(Assignee)

Comment 47

5 years ago
Comment on attachment 8366178 [details] [review]
https://github.com/mozilla/MakeAPI/pull/189

Ooops, forgot to bring those fixes over too from the previous patch.
Attachment #8366178 - Flags: review?(jon)
Attachment #8366178 - Flags: review?(cade)
Attachment #8366178 - Flags: review-
Comment on attachment 8366178 [details] [review]
https://github.com/mozilla/MakeAPI/pull/189

This doesn't need to happen here - it'll happen in Bug 922166
(Assignee)

Comment 49

5 years ago
Well feel free to close this bug then.

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 922166

Updated

5 years ago
Attachment #8366178 - Flags: review?(jon)
Attachment #8366178 - Flags: review?(cade)

Updated

5 years ago
No longer blocks: 912696
You need to log in before you can comment on or make changes to this bug.