Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reshares_count come back! Fix #3370 #3700

Merged
merged 1 commit into from
Nov 8, 2012
Merged

Reshares_count come back! Fix #3370 #3700

merged 1 commit into from
Nov 8, 2012

Conversation

movilla
Copy link
Contributor

@movilla movilla commented Nov 3, 2012

In previous episodes... #3699
#3370

@jhass
Copy link
Member

jhass commented Nov 3, 2012

Again, you can just update an existing pull request by (force) pushing to the same branch, no need to open a myriad of pull requests for each feature/bug ;). For example to edit the last commit, checkout the existing branch, make the changes and run git commit -a --amend afterwards (or git add and git commit --amend without the -a to include only some changes). Or if you have multiple commits and want to edit an older one, run git rebase -i develop, change the one you want to edit to edit, close the editor and then you'll be informed that you can edit the commit now in the same way you would edit the last commit. Once you're done with that run git rebase --continue to return back into a normal state or git rebase --abort at any time to abort without doing any change. In either case you can then update a pull request by a simple git push -f origin yourbranchname. Would make it easier to follow the discussion and everything :)

Now sadly Travis is unhappy about this change, can you take a look?

@movilla
Copy link
Contributor Author

movilla commented Nov 3, 2012

Sorry, I'm a stubborn :[

Diabolic-Travis now is happy

this.interactions.reshare();
expect(this.userReshare.change).not.toHaveBeenCalled();
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this one confuses me, the description says the opposite of what's tested. Since it passes, what's the desired behaviour now?

@movilla
Copy link
Contributor Author

movilla commented Nov 5, 2012

I understand. The description should be calls no change on the reshare collection.

I added this test to see that for reshare: function () did haven't a test. But I think the major change to the test, was added this https://github.com/diaspora/diaspora/pull/3700/files#L4R31 and https://github.com/diaspora/diaspora/pull/3700/files#L4R38

@jhass
Copy link
Member

jhass commented Nov 6, 2012

I'm a backbone noob, so what would it mean if it's called? What's the effect and why is it undesired?

@movilla
Copy link
Contributor Author

movilla commented Nov 6, 2012

I'm a ALL noob :] . I understand that the author is unable to reshare the publication

@Raven24
Copy link
Member

Raven24 commented Nov 6, 2012

I think you need to rebase now, since the changelog was altered by another PR.
also, I think you can remove that one test you were talking about. It doesn't really test anything important.

And, you introduced a syntax error in the spec file... you need to change it like so:

--- a/spec/javascripts/app/views/stream_post_spec.js
+++ b/spec/javascripts/app/views/stream_post_spec.js
@@ -28,19 +28,19 @@ describe("app.views.StreamPost", function(){

     context("reshare", function(){
       it("displays a reshare count", function(){
-        this.statusMessage.set({interactions.reshares_count : 2})
+        this.statusMessage.set({ interactions: {reshares_count : 2 }});
         var view = new this.PostViewClass({model : this.statusMessage}).render();

-        expect($(view.el).html()).toContain(Diaspora.I18n.t('stream.reshares', {count: 2}))
-      })
+        expect($(view.el).html()).toContain(Diaspora.I18n.t('stream.reshares', {count: 2}));
+      });

       it("does not display a reshare count for 'zero'", function(){
-        this.statusMessage.set({interactions.reshares_count : 0})
+        this.statusMessage.interactions.set({ interactions: { reshares_count : 0}} );
         var view = new this.PostViewClass({model : this.statusMessage}).render();

-        expect($(view.el).html()).not.toContain("0 Reshares")
-      })
-    })
+        expect($(view.el).html()).not.toContain("0 Reshares");
+      });
+    });

     context("likes", function(){
       it("displays a like count", function(){

@movilla
Copy link
Contributor Author

movilla commented Nov 6, 2012

Oks thank you. I'm on it.

@movilla
Copy link
Contributor Author

movilla commented Nov 6, 2012

Sorry about the mess O-O

She's ready for the dance.

@DeadSuperHero
Copy link
Member

"This pull request cannot be automatically merged."

Sad face. For what it's worth, I can test this on my localpod, and provided that everything works, I can merge it for you. Would love to see reshare counts to return to the stream.

@DeadSuperHero
Copy link
Member

Works great, merging. Thanks Movilla! :D

@DeadSuperHero DeadSuperHero merged commit 5a2b22f into diaspora:develop Nov 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants