Fixing #2479: Allow user to change the project description without needing to publish their project. #2528

jmrodriguesgoncalves · 2017-10-14T04:13:30Z

Description wasn't saving if the project wasn't published. Now, by clicking cancel while the project is unpublished, the description is saved. Please review this and let me know if there's something that can be fixed.

Description wasn't saving if the project wasn't published. Now, by clicking cancel while the project is unpublished, the description is saved

flukeout · 2017-10-16T19:47:47Z

Vagrantfile

@@ -11,7 +11,7 @@ Vagrant.configure("2") do |config|
 # VM, but users are encouraged to test this and make adjustments below (or file PRs)
 # if you find the VM lagging or unresponsive.
 config.vm.provider "virtualbox" do |v|
- v.memory = 1536
+ v.memory = 8000


Does this change need to be part of this PR?

Not at all, I should actually have left that as it was, that's something I forgot to change. When testing, the loading between pages took a little too long so I thought giving the VM a bunch of space would make things faster on my end. It didn't change anything, and it slipped right by me. We can change it to the way it was before.

Okay yeah, please change it back, thanks!

flukeout · 2017-10-16T21:01:27Z

OK - thanks for submitting this PR @jmrodriguesgoncalves!

My suggestion for this is to save the description when someone clicks outside of the Publish dialog and the dialog closes it. Currently, if I change the description and then click outside of the popup dialog, the change is lost. Hitting the Cancel button doesn't feel like it should save the description, which it does currently. So, in summary I'd recommend...

Save the new description if someone clicks outside of the publish dialog
Throw away the description of someone clicks the Cancel button

Let me know your thoughts!

jmrodriguesgoncalves · 2017-10-16T21:14:13Z

I think that sounds good, I can try to do what you are asking me. Clicking outside would save the description as you said, so we'd use the same code we have now, except, when clicking anywhere outside the Publish dialog.

Now, if we click the cancel button, should the description be completely erased, as in, making it blank, or should we have it be the way it was before, which basically kept the string intact while editing the project (so one would still see it there if they were to open the publish dialog again), but not saved if not published (so it would not show up when looking at their list of projects, or if closed and opened again for editing)?

Changed Vagrant settings back to normal. I also simplified the code and made it clearer as to what the code is currently doing. Guidance is needed for ajax request, as well as how to interact with the outside of the Publish dialog, in order to change how the description is saved (instead of clicking cancel, one would have to click outside the dialog).

flukeout · 2017-10-17T18:38:04Z

Yeah the Cancel case is a bit strange for us. I just thought of a great solution however...

What if we just remove the Cancel button from the UI altogether? That way, you HAVE to click outside of the popup to close it, at which point we'll just save whatever changes you made.

The other little popups work this way already, the settings panel and file dropdown for example.

What do you think @jmrodriguesgoncalves ? Simplifies things a bit for us.

jmrodriguesgoncalves · 2017-10-17T18:46:39Z

@flukeout I think that is a great idea that not only would simplify the code, but would also be much less confusing for the end user.

If we are to go ahead with this idea, I have a quick question for you since you have more familiarity with this code. Just like we talked about on the chat, I am struggling to find the function that deals with the click outside of the dialog box. I have tried setting an event listener for the div under it, "#click-underlay", however that does not seem to work.

If you have any input and if we are to go ahead with the UI change as well, let me know and I will get working on it. Thanks again!

flukeout · 2017-10-17T19:34:41Z

Hey @jmrodriguesgoncalves - yeah let's def go ahead with removing the Cancel button then. I'll dig into the code and look for the click-underlay stuff

… into p1

flukeout · 2017-10-17T19:50:26Z

OK @jmrodriguesgoncalves - I think I found what we need.

Check out this line here.

That's where hidePublishDialog() is defined, which is then passed into the new Underlay constructor on line 448 - when the Underlay is clicked, it removes the the underlay and fires that function - I tested this briefly with a console.log to make sure it works. I think you can add your logic there. Hope that helps - let me know.

commented out Cancel button, added non-working click-underlay logic (commented out, may help with review)

jmrodriguesgoncalves · 2017-10-17T19:53:49Z

@flukeout great stuff, never mind the latest 2 pushes, I will try this!

flukeout · 2017-10-17T19:59:09Z

Yeah do what you need, you can just keep updating this PR, we can squash it all into one commit when we merge it.

Cancel button has been removed from UI. To save the description, all a user needs to do is click outside the dialog box. This works whether the project is published or unpublished.

jmrodriguesgoncalves · 2017-10-17T21:27:20Z

@flukeout So currently, the code does everything that we decided to go with. It saves the description when the user clicks outside of the dialog box, and since the cancel button isn't there anymore, the user won't be as confused.

On the back-end however, the solution I used to make this work may not be the most elegant. In order to save the description, the function checks whether or not the project the user is working on is currently published or not. If it isn't, an ajax request is made to "unpublish" the unpublished project. This of course throws an error message, but the description is saved perfectly. If the project is published however, the code attempts an ajax request to publish with that same ID it is published on, changing only the description, which means nothing but that field will change. What this does is basically allow the user to change the description whether or not their project is published.

flukeout · 2017-10-17T22:41:01Z

Hey awesome @jmrodriguesgoncalves - I'll test out the functionality now, and let's ping @gideonthomas so he can provide his thoughts on the back-end piece you asked about.

Edit: OK—I confirmed that the expected behaviour is happening! Nice.

humphd · 2017-10-18T14:10:54Z

@jmrodriguesgoncalves looks like there are some things you can fix in publisher.js, see https://travis-ci.org/mozilla/thimble.mozilla.org/builds/289235540#L483

Spacing

Fixing spacing issues

Code cleanup

flukeout · 2017-10-18T21:54:18Z

@jmrodriguesgoncalves let us know when this is ready to re-test!

jmrodriguesgoncalves · 2017-10-18T22:09:14Z

@humphd got them sorted out, there were some spacing issues!

@flukeout, I haven't changed the logic from what you have tested so there shouldn't be any surprises, but let me know! This latest commit has the same functionality as before, just the spacing is now done correctly as to pass the two tests from below

flukeout · 2017-10-18T22:52:14Z

OK, I'll wait on @humphd to weigh in on the changes you made and then I'll do one last functional test before we get this landed. Cheers!

gideonthomas

Sorry for getting to this so late. @jmrodriguesgoncalves, I added some suggestions. I would advocate for not forcing a publish just to update the description, even as a quick fix, because the sheer number of requests will kill the database.

Would you also be able to rebase your changes as well?

Let us know if you have any questions.

gideonthomas · 2017-10-20T16:25:36Z

public/editor/scripts/ui/publisher.js

+//throws error since nothing is being unpublished, however this works
+//to save description. This is currently a "hack", a more elegant
+//solution may be implemented after
+Publisher.prototype.setDescription = function() {


let's rename this to saveDescription

gideonthomas · 2017-10-20T17:39:41Z

public/editor/scripts/ui/publisher.js

+//to save description. This is currently a "hack", a more elegant
+//solution may be implemented after
+Publisher.prototype.setDescription = function() {
+ var publisher = this;


Let's not force a "publish" just to save the description. Instead, let's make the following changes:

To this function:

const publisher = this; const oldDescription = Project.getDescription(); const description = publisher.dialog.description.val(); if(oldDescription === description) { return; } Project.setDescription(description); const data = { title: Project.getTitle(), description, dateUpdated: new Date().toISOString() }; Metadata.update({ update: true, csrfToken: publisher.csrfToken, host: Project.getHost(), id: Project.getId(), data }, error => { console.error("[Thimble] Failed to update project description with: ", error); });

you'll also need to require in the metadata module from https://github.com/mozilla/thimble.mozilla.org/blob/master/public/editor/scripts/project/metadata.js

This does not to work, even with the metadata module included. After inspecting, it seems this is producing a bad request.

gideonthomas · 2017-10-20T17:39:51Z

public/editor/scripts/ui/publisher.js

@@ -283,6 +328,7 @@ Publisher.prototype.unpublish = function() {
 request.always(function() {
 SyncState.completed();
 publisher.unpublishing = false;
+ publickCheck = false;


what's this?

Nothing, it has been removed.

WIP

Spacing

Travis CI spacing

Fixing #2479

676235b

Description wasn't saving if the project wasn't published. Now, by clicking cancel while the project is unpublished, the description is saved

gideonthomas self-requested a review October 14, 2017 17:03

flukeout reviewed Oct 16, 2017

View reviewed changes

Merge branch 'master' of https://github.com/mozilla/thimble.mozilla.org…

129191a

… into p1

Fixing #2479 WIP

9f58b2b

commented out Cancel button, added non-working click-underlay logic (commented out, may help with review)

Fixing #2479

cae4f1d

Cancel button has been removed from UI. To save the description, all a user needs to do is click outside the dialog box. This works whether the project is published or unpublished.

jmrodriguesgoncalves added 3 commits October 18, 2017 17:03

Fixing #2479

b923552

Spacing

Fix #2479

4de70ee

Fixing spacing issues

Fixing #2479

6f30723

Code cleanup

gideonthomas suggested changes Oct 20, 2017

View reviewed changes

jmrodriguesgoncalves added 5 commits October 20, 2017 15:40

Fixing #2479

10b8786

WIP

Fixing #2479 WIP

db1a1ee

Spacing

Fixing #2479

fbf8384

Fixing #2479

2b0ce8a

Travis CI spacing

Fixing #2479 - Travis CI

b9a8f69

Fixing #2479 - Travis CI

91b7f57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing #2479: Allow user to change the project description without needing to publish their project. #2528

Fixing #2479: Allow user to change the project description without needing to publish their project. #2528

jmrodriguesgoncalves commented Oct 14, 2017

flukeout Oct 16, 2017

jmrodriguesgoncalves Oct 16, 2017

flukeout Oct 16, 2017

flukeout commented Oct 16, 2017

jmrodriguesgoncalves commented Oct 16, 2017

flukeout commented Oct 17, 2017 •

edited

jmrodriguesgoncalves commented Oct 17, 2017 •

edited

flukeout commented Oct 17, 2017

flukeout commented Oct 17, 2017

jmrodriguesgoncalves commented Oct 17, 2017

flukeout commented Oct 17, 2017

jmrodriguesgoncalves commented Oct 17, 2017

flukeout commented Oct 17, 2017 •

edited

humphd commented Oct 18, 2017

flukeout commented Oct 18, 2017

jmrodriguesgoncalves commented Oct 18, 2017

flukeout commented Oct 18, 2017

gideonthomas left a comment

gideonthomas Oct 20, 2017

gideonthomas Oct 20, 2017

gideonthomas Oct 20, 2017

jmrodriguesgoncalves Oct 20, 2017

gideonthomas Oct 20, 2017

jmrodriguesgoncalves Oct 20, 2017

Fixing #2479: Allow user to change the project description without needing to publish their project. #2528

Are you sure you want to change the base?

Fixing #2479: Allow user to change the project description without needing to publish their project. #2528

Conversation

jmrodriguesgoncalves commented Oct 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flukeout commented Oct 16, 2017

jmrodriguesgoncalves commented Oct 16, 2017

flukeout commented Oct 17, 2017 • edited

jmrodriguesgoncalves commented Oct 17, 2017 • edited

flukeout commented Oct 17, 2017

flukeout commented Oct 17, 2017

jmrodriguesgoncalves commented Oct 17, 2017

flukeout commented Oct 17, 2017

jmrodriguesgoncalves commented Oct 17, 2017

flukeout commented Oct 17, 2017 • edited

humphd commented Oct 18, 2017

flukeout commented Oct 18, 2017

jmrodriguesgoncalves commented Oct 18, 2017

flukeout commented Oct 18, 2017

gideonthomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flukeout commented Oct 17, 2017 •

edited

jmrodriguesgoncalves commented Oct 17, 2017 •

edited

flukeout commented Oct 17, 2017 •

edited