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.

Sign up for GitHub

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

Jump to bottom

ADD: Keyboard shortcut to publish a comment and tests for it #7301

Merged
merged 3 commits into from
Jan 21, 2020
Merged

ADD: Keyboard shortcut to publish a comment and tests for it #7301

merged 3 commits into from
Jan 21, 2020

Conversation

VladimirMikulic
Copy link
Contributor

Pressing CTRL/CMD + Enter will publish a comment.

Comment tests:

  • CTRL/CMD + enter comment publishing keyboard shortcut
  • comment deletion

Resolves #5193 & #5473

@VladimirMikulic VladimirMikulic requested a review from a team as a code owner January 19, 2020 04:21
@codecov
Copy link

codecov bot commented Jan 19, 2020 •
edited
Loading

Codecov Report

Merging #7301 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7301   +/-   ##
=======================================
  Coverage   81.61%   81.61%           
=======================================
  Files          97       97           
  Lines        5602     5602           
=======================================
  Hits         4572     4572           
  Misses       1030     1030

@Uzay-G
Copy link
Member

Uzay-G commented Jan 19, 2020 •
edited
Loading

I wonder why that system test is failing. I made it in a pr recently and everything worked fine. It also worked in #7291 and #7292

@VladimirMikulic
Copy link
Contributor Author

Don't worry, the update is coming.

@Uzay-G
Copy link
Member

Uzay-G commented Jan 19, 2020

Oh ok, how does this pr affect that test though? I am just curious 👍

@codecov
Copy link

codecov bot commented Jan 19, 2020

Codecov Report

Merging #7301 into master will decrease coverage by 29.84%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #7301       +/-   ##
===========================================
- Coverage    81.6%   51.76%   -29.85%     
===========================================
  Files          97       97               
  Lines        5600     6819     +1219     
===========================================
- Hits         4570     3530     -1040     
- Misses       1030     3289     +2259
Impacted Files Coverage Δ
app/models/user.rb 87.93% <100%> (-3.08%) ⬇️
app/models/drupal_content_field_mapper.rb 100% <0%> (ø) ⬆️
app/channels/application_cable/connection.rb 100% <0%> (ø) ⬆️
app/channels/application_cable/channel.rb 100% <0%> (ø) ⬆️
app/mailers/password_reset_mailer.rb 100% <0%> (ø) ⬆️
app/models/drupal_content_field_map_editor.rb 100% <0%> (ø) ⬆️
app/services/execute_search.rb 88.88% <0%> (-5.56%) ⬇️
app/controllers/user_tags_controller.rb 84.28% <0%> (ø) ⬆️
app/channels/user_notification_channel.rb 83.33% <0%> (ø) ⬆️
app/channels/user_channel.rb 83.33% <0%> (ø) ⬆️
... and 43 more

@VladimirMikulic
Copy link
Contributor Author

@Uzay-G I was deleting the only comment on the post instead of creating my own comment and deleting it. Now it works.

@Uzay-G
Copy link
Member

Uzay-G commented Jan 19, 2020

@Uzay-G I was deleting the only comment on the post instead of creating my own comment and deleting it. Now it works.

Oh ok. The pr looks good. I will test it locally

@SidharthBansal
Copy link
Member

It will be great if you create two prs
First for enter and cmd shortcut(add and edit comment test)
Second for delete
This will give you 10 points.

@VladimirMikulic
Copy link
Contributor Author

Thanks, @SidharthBansal.
I've decided to include them in the same PR because I wouldn't be able to test the feature if there is no feature 😄

It's not a big change, so it shouldn't be a problem to review.
I'll also add comment edit test, good catch 👍

@SidharthBansal
Copy link
Member

SidharthBansal commented Jan 20, 2020 via email

@VladimirMikulic
Copy link
Contributor Author

Thank you very much @SidharthBansal, but this is system tests part 3.
Together with #7306, I have 5 tests.

@SidharthBansal
Copy link
Member

Once the changes are done, please get it approved by Uzay so that I can quickly merge this. It is better to have two reviews than one.
Thanks

@SidharthBansal
Copy link
Member

Just a short note, I am constantly asking Uzay to review your prs and you to review theirs. Do you know why? Have you heard about pair programming? One is remote and other is driver. By this I want you both to learn how to build a professional pairing relation. This is the first activity with which collaboration starts. Please read two three articles about pair programming. (I will not give extra points for reading those articles....lol)
More software engineering stuff soon folks

@SidharthBansal
Copy link
Member

SidharthBansal commented Jan 20, 2020 via email

@VladimirMikulic
Copy link
Contributor Author

Fair enough, which portion of the codebase do you want me to test next?
Maybe posting a question, there we have only a screenshot test.

@VladimirMikulic
Copy link
Contributor Author

@Uzay-G could you take a look at this? Thanks.

@SidharthBansal
Copy link
Member

SidharthBansal commented Jan 20, 2020 via email

@VladimirMikulic
Copy link
Contributor Author

VladimirMikulic commented Jan 20, 2020 •
edited
Loading

Absolutely! By the way, I resolved the issues with most of the places that you've pinged me.
I bookmark them carefully :)

@SidharthBansal
Copy link
Member

SidharthBansal commented Jan 20, 2020 via email

@SidharthBansal
Copy link
Member

SidharthBansal commented Jan 20, 2020 via email

@VladimirMikulic
Copy link
Contributor Author

Oh, how do you bookmark?

Nothing special, I just star the email and unstar it once the issue has been resolved.

Also it will be great if you can open system tests prs for many remaining
portions like questions stats etc.

No problem. First I'll figure out what needs to be tested and then get to work 👍

@SidharthBansal
Copy link
Member

SidharthBansal commented Jan 20, 2020 via email

@VladimirMikulic
Copy link
Contributor Author

VladimirMikulic commented Jan 20, 2020 •
edited
Loading

The changes updated accordingly.

@SidharthBansal
Copy link
Member

SidharthBansal commented Jan 20, 2020 via email

@SidharthBansal
Copy link
Member

SidharthBansal commented Jan 20, 2020 via email

@VladimirMikulic
Copy link
Contributor Author

It's ready.

@SidharthBansal
Copy link
Member

SidharthBansal commented Jan 20, 2020 via email

@VladimirMikulic
Copy link
Contributor Author

VladimirMikulic commented Jan 20, 2020 •
edited
Loading

@SidharthBansal this breaks Travis. I suggest that we keep keypress.
The test is complicated by itself, as you can see it's hard to fake a browser event.

That's why I put the keypress in the first place, it ignores CTRL and when user presses the second key (could be Enter in our case) it triggers the comment upload.

@SidharthBansal
Copy link
Member

Please fill the grade sheet and mentor evaluation forms

@SidharthBansal
Copy link
Member

Please get all tasks approved asap. Even if PR isn't merged but it is present then also I will approve the task so that you can complete it till the deadline. Don't want to miss giving marks for any task

@VladimirMikulic
ADD: Shortcut to publish a comment and tests
ebbd6fc 
Pressing CTRL/CMD + Enter will publish a comment.

Comment tests:
 - CTRL/CMD + enter comment publishing keyboard shortcut
 - comment deletion

Resolves #5193 & #5473
@VladimirMikulic
ADD: Comment editing system test
f31e670
@VladimirMikulic
FIX: post_test reverting wiki versions
8c5217e
@VladimirMikulic
Copy link
Contributor Author

@SidharthBansal this is ready to be merged and will fix our failing build.
I think I've discovered why the test failed again.

We were clicking revert button and immediately after it going to to the wiki(visit).
The post request sent by "Revert" would be interrupted and it would result in a test failure.

Calling visit after clicking "Revert" was the problem since clicking "Revert" will return you to wiki anyways. Now Travis should be healthy again :)

@SidharthBansal
Copy link
Member

SidharthBansal commented Jan 21, 2020 via email

@VladimirMikulic
Copy link
Contributor Author

VladimirMikulic commented Jan 21, 2020 •
edited
Loading

@jywarren @cesswairimu could you take a look at this and merge it? Thanks.

@jywarren
Copy link
Member

Looks awesome. Thanks, all!!! 🎉

@jywarren
Copy link
Member

Also will add Sidharth to the correct maintainers groups. Still learning new approval workflow from #6501 !

@jywarren jywarren merged commit 450474e into publiclab:master Jan 21, 2020
@VladimirMikulic VladimirMikulic mentioned this pull request Jan 30, 2020
Closed
5 tasks
vinitshahdeo pushed a commit to vinitshahdeo/plots2 that referenced this pull request Feb 1, 2020
@VladimirMikulic
ADD: Keyboard shortcut to publish a comment and tests for it (publicl…
be1dc8f 
…ab#7301)

* ADD: Shortcut to publish a comment and tests

Pressing CTRL/CMD + Enter will publish a comment.

Comment tests:
 - CTRL/CMD + enter comment publishing keyboard shortcut
 - comment deletion

Resolves publiclab#5193 & publiclab#5473

* ADD: Comment editing system test

* FIX: post_test reverting wiki versions
NitinBhasneria pushed a commit to NitinBhasneria/plots2 that referenced this pull request Feb 5, 2020
@VladimirMikulic
ADD: Keyboard shortcut to publish a comment and tests for it (publicl…
760b4fe 
…ab#7301)

* ADD: Shortcut to publish a comment and tests

Pressing CTRL/CMD + Enter will publish a comment.

Comment tests:
 - CTRL/CMD + enter comment publishing keyboard shortcut
 - comment deletion

Resolves publiclab#5193 & publiclab#5473

* ADD: Comment editing system test

* FIX: post_test reverting wiki versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Uzay-G Uzay-G Uzay-G left review comments

@SidharthBansal SidharthBansal SidharthBansal approved these changes

@jywarren jywarren Awaiting requested review from jywarren

@emilyashley emilyashley Awaiting requested review from emilyashley

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add shortcut keys for rich legacy editor
4 participants
@VladimirMikulic @Uzay-G @SidharthBansal @jywarren