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

[issue - 28, 67, 68, 72,73] improvements in bitbucket subscriptions #75

Closed

Conversation

sibasankarnayak
Copy link
Contributor

@sibasankarnayak sibasankarnayak commented Apr 5, 2022

made some improvements in logic for subscription

Ticket Here
Fixes #67
Fixes #68
Fixes #72
Fixes #73

@mattermod
Copy link
Contributor

Hello @sibasankarnayak,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@sibasankarnayak
Copy link
Contributor Author

@dipak-demansol Can you start the QA in mean time to crosscheck if i had covered all the logical cases.

@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Apr 5, 2022
@hanzei hanzei added this to the v1.1.2 milestone Apr 5, 2022
Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

@sibasankarnayak Can you please remove the changes pertaining to attaching subscription to its creator? It was determined in #28 that this new behavior is undesired.

As long as "regular" users cannot see or edit subscriptions in channels they are not a member of, the current behavior is fine.

@dipak-demansol
Copy link

dipak-demansol commented Apr 6, 2022

@sibasankarnayak i'll start working on after you Complete the changes as per mickmister Comment.

@sibasankarnayak
Copy link
Contributor Author

dit subscriptions in channels they are not a member of, the current behavior is fine.

@sibasankarnayak Can you please remove the changes pertaining to attaching subscription to its creator? It was determined in #28 that this new behavior is undesired.

As long as "regular" users cannot see or edit subscriptions in channels they are not a member of, the current behavior is fine.

@mickmister Sure, have seperated the code for issue #28

@dipak-demansol you can start testing

sibasankarnayak added 2 commits April 6, 2022 15:15
@hanzei hanzei requested a review from mickmister April 6, 2022 16:29
Copy link

@dipak-demansol dipak-demansol left a comment

Choose a reason for hiding this comment

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

@sibasankarnayak
:- #67 :- LGTM
:- #72 :- LGTM
:- #73 :- LGTM
:- #68 :- have minor issue, issue link https://drive.google.com/file/d/1A68jP9Ig-6xjP8ejMfBxY2SBNCKDFQFx/view?usp=sharing

sibasankarnayak added 2 commits April 8, 2022 16:56
@sibasankarnayak
Copy link
Contributor Author

@sibasankarnayak :- #67 :- LGTM :- #72 :- LGTM :- #73 :- LGTM :- #68 :- have minor issue, issue link https://drive.google.com/file/d/1A68jP9Ig-6xjP8ejMfBxY2SBNCKDFQFx/view?usp=sharing

@dipak-demansol can you retest once more , fixed the logic

@dipak-demansol dipak-demansol self-requested a review April 9, 2022 07:50
Copy link

@dipak-demansol dipak-demansol left a comment

Choose a reason for hiding this comment

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

LGTM

@dipak-demansol dipak-demansol added QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels Apr 9, 2022
@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

server/command.go Show resolved Hide resolved
server/command.go Show resolved Hide resolved
server/subscriptions.go Outdated Show resolved Hide resolved
server/subscriptions.go Outdated Show resolved Hide resolved
server/subscriptions.go Outdated Show resolved Hide resolved
@hanzei hanzei added the Awaiting Submitter Action Blocked on the author label Apr 30, 2022
@sibasankarnayak sibasankarnayak removed the Awaiting Submitter Action Blocked on the author label Apr 30, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 30, 2022

Codecov Report

Merging #75 (ba1bf81) into master (806f094) will increase coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   16.18%   16.26%   +0.07%     
==========================================
  Files          13       13              
  Lines        1853     1844       -9     
==========================================
  Hits          300      300              
+ Misses       1534     1525       -9     
  Partials       19       19              
Impacted Files Coverage Δ
server/command.go 7.11% <0.00%> (+0.32%) ⬆️
server/subscriptions.go 14.17% <0.00%> (-0.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 806f094...ba1bf81. Read the comment docs.

server/subscriptions.go Outdated Show resolved Hide resolved
server/command.go Outdated Show resolved Hide resolved
@mickmister mickmister requested a review from jfrerich May 6, 2022 18:30
@hanzei
Copy link
Collaborator

hanzei commented Oct 14, 2022

@mickmister @jfrerich Can we merge this PR? (After resolving the conflicts)

@jfrerich
Copy link
Contributor

@hanzei, I don't see why we wouldn't.

@mickmister
Copy link
Contributor

Can we merge this PR? (After resolving the conflicts)

@hanzei This PR was submitted by Demansol. I'm not sure if we're able to update the PR with the conflict resolutions

@hanzei
Copy link
Collaborator

hanzei commented Nov 8, 2022

Re-submitted as #97

@hanzei hanzei closed this Nov 8, 2022
@hanzei hanzei added Duplicate This issue or pull request already exists and removed 2: Dev Review Requires review by a core committer Lifecycle/1:stale QA Review Done PR has been approved by QA Triage labels Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment