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

fix the bug of traceback error of server on webpage #5402

Merged
merged 2 commits into from
Apr 24, 2019
Merged

fix the bug of traceback error of server on webpage #5402

merged 2 commits into from
Apr 24, 2019

Conversation

CleverFool77
Copy link
Member

@CleverFool77 CleverFool77 commented Apr 7, 2019 •
edited
Loading

Fixes #5378 #5404<=== Add issue number here)

The changes -

  • If the user adds with tag like with,then there won't be a traceback error like earlier
  • If the user adds with: or barnstars: tag, then it won't show the traceback error rather it would show that the power tag can't be empty.
  • The tagname is stripped of whitepaces in ends. As for admin, the code doesn't check condition to find username rather adds it directly. So In such cases the whitespace might be added.

Before changes
error

After changes
ff

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

Thanks!

@CleverFool77 CleverFool77 changed the title fix the traceback error of server on webpage fix the bug of traceback error of server on webpage Apr 7, 2019
@plotsbot
Copy link
Collaborator

plotsbot commented Apr 7, 2019 •
edited
Loading

1 Message
📖 @CleverFool77 Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

@CleverFool77
Copy link
Member Author

Hi @jywarren @gauravano ,
while working on this issue.
I found some bugs, for which I've opened a new issue #5404

grvsachdeva
grvsachdeva reviewed Apr 8, 2019
View reviewed changes
app/controllers/tag_controller.rb Outdated
@@ -300,7 +300,7 @@ def create
body: "@#{current_user.username} awards a #{barnstar_info_link} to #{node.user.name} for their awesome contribution!")

elsif tagname.split(':')[0] == "with"
user = User.find_by_username_case_insensitive(tagname.split(':')[1])
user = User.find_by_username_case_insensitive(current_user.username)
Copy link
Member

Choose a reason for hiding this comment

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

Power-tag with:user1 adds user with username user1 as co-author of the note. So, we want to get their username from tag and so this change is wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh!!! I missed out this. Sorry for the wrong change. I'll commit with correct one soon.
Thanks @gauravano

@CleverFool77
Copy link
Member Author

CleverFool77 commented Apr 9, 2019 •
edited
Loading

Hi @gauravano @jywarren I've updated the PR as requested. 😄
Thanks !!!!!

@CleverFool77 CleverFool77 force-pushed the traceback-error branch 2 times, most recently from 814e234 to ee53cbf Compare April 12, 2019 05:45
@CleverFool77
Copy link
Member Author

@publiclab/plots2-reviewers
Please Review.
Thanks !!

grvsachdeva
grvsachdeva reviewed Apr 15, 2019
View reviewed changes
app/controllers/tag_controller.rb Outdated
if Tag.exists?(tagname, nid)
@output[:errors] << I18n.t('tag_controller.tag_already_exists')

elsif tagname.include?(":") && tagname.split(':').length < 2 # cant tag empty power tags
Copy link
Member

Choose a reason for hiding this comment

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

You mean Can't add empty power tags 😅 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @gauravano
What I mean was we can add with but not with: ? 😅
Is this not supposed to be like this ?

Copy link
Member

Choose a reason for hiding this comment

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

I was referring to comment not code 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh!! Forgot about that. I'll fix it.

grvsachdeva
grvsachdeva reviewed Apr 15, 2019
View reviewed changes
app/controllers/tag_controller.rb
if Tag.exists?(tagname, nid)
@output[:errors] << I18n.t('tag_controller.tag_already_exists')

elsif tagname.include?(":") && tagname.split(':').length < 2 # cant tag empty power tags
if tagname.split(':')[0] == "barnstar" || tagname.split(':')[0] == "with"
Copy link
Member

Choose a reason for hiding this comment

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

hey @CleverFool77, I think we can implement this for all power tags, we have many - https://publiclab.org/wiki/power-tags. What do you say?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @gauravano In other power tags, If I'll add some of the other power tags like build:, alert: ,notes: etc then neither it shows traceback error nor its gets added, which is correct.

Only with: and barnstars: which were implemented in different way were showing traceback error and That's why I added only both of them here.
Thanks !!!

Copy link
Member

Choose a reason for hiding this comment

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

Ok! I think, we can discuss it after merging this that whether we want to show alert if someone try to use incomplete power tag, even in case where no error is thrown. Thanks!

@grvsachdeva
Copy link
Member

grvsachdeva commented Apr 15, 2019 •
edited
Loading

Great work here! @CleverFool77 💯 👍

@grvsachdeva
Copy link
Member

I think, we are almost done here. Just correct the comment or you can also remove it. Thanks!

@CleverFool77
Copy link
Member Author

Hi @gauravano
I've updated the PR.
Thanks !!

grvsachdeva
grvsachdeva approved these changes Apr 24, 2019
View reviewed changes
Copy link
Member

@grvsachdeva grvsachdeva left a comment

Choose a reason for hiding this comment

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

Great work!

@grvsachdeva grvsachdeva merged commit 22fe71c into publiclab:master Apr 24, 2019
@grvsachdeva grvsachdeva removed the ready label Apr 24, 2019
@grvsachdeva grvsachdeva mentioned this pull request Apr 24, 2019
Closed
@grvsachdeva
Copy link
Member

🎉 💯 👍

SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
@CleverFool77 @grvsachdeva
fix the bug of traceback error of server on webpage (publiclab#5402)
7672ac9 
* fix the bug of traceback error of server on webpage

* remove comment
digitaldina pushed a commit to digitaldina/plots2 that referenced this pull request May 12, 2019
@CleverFool77 @digitaldina
fix the bug of traceback error of server on webpage (publiclab#5402)
1b01aa0 
* fix the bug of traceback error of server on webpage

* remove comment
@CleverFool77 CleverFool77 deleted the traceback-error branch June 28, 2019 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grvsachdeva grvsachdeva grvsachdeva approved these changes

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

Successfully merging this pull request may close these issues.

traceback of error from server is shown in webpage
3 participants
@CleverFool77 @plotsbot @grvsachdeva