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

Slack Work #3232

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Slack Work #3232

wants to merge 20 commits into from
+297 −0

Conversation

Dhruv-Sharma01
Copy link

@Dhruv-Sharma01 Dhruv-Sharma01 commented Jan 16, 2025 •
edited
Loading

Added a csv of project slack channels of OWASP in project_channels.csv.
Also created a new field "slack" in project model(models.py) and did changes in project_detail.html to view slack channel.
fixes #3198

DonnieBLT
DonnieBLT requested changes Jan 16, 2025
View reviewed changes
Copy link
Collaborator

@DonnieBLT DonnieBLT left a comment

Choose a reason for hiding this comment

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

Can you check this again please there seems to be some overlap with code

@yashpandey06
Copy link

yashpandey06 commented Jan 16, 2025 •
edited
Loading

Tests for these changes have been added ?

@Dhruv-Sharma01

@JisanAR03
Copy link
Contributor

can you please review the project.py file cause u have pushed few unused code in that file

@Dhruv-Sharma01
Copy link
Author

@yashpandey06 I am not sure about it.

@Dhruv-Sharma01
Copy link
Author

can you please review the project.py file cause u have pushed few unused code in that file

Will do that.

@Dhruv-Sharma01
Copy link
Author

Can you check this again please there seems to be some overlap with code

Sir, I have commited the changes in my PR please check once.

DonnieBLT
DonnieBLT requested changes Jan 19, 2025
View reviewed changes
Copy link
Collaborator

@DonnieBLT DonnieBLT left a comment

Choose a reason for hiding this comment

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

lets add slack_channel, slack_id, and slack_url

@Dhruv-Sharma01
Copy link
Author

lets add slack_channel, slack_id, and slack_url

Done Sir, please check.

DonnieBLT
DonnieBLT requested changes Jan 19, 2025
View reviewed changes
website/templates/projects/project_detail.html Outdated Show resolved Hide resolved
website/templates/projects/repo_detail.html Outdated Show resolved Hide resolved
@DonnieBLT
Copy link
Collaborator

And please add them to the models and create an import management command

Dhruv-Sharma01 and others added 2 commits January 19, 2025 21:14
@Dhruv-Sharma01 @DonnieBLT
Update website/templates/projects/repo_detail.html
44273ac 
Co-authored-by: DonnieBLT <128622481+DonnieBLT@users.noreply.github.com>
@Dhruv-Sharma01 @DonnieBLT
Update website/templates/projects/project_detail.html
955af6e 
Co-authored-by: DonnieBLT <128622481+DonnieBLT@users.noreply.github.com>
@Dhruv-Sharma01
Copy link
Author

And please add them to the models and create an import management command

Sir, please let me know if anything else is needed.

@DonnieBLT
Copy link
Collaborator

Instead of a new model can you please put those fields on the project?

@Dhruv-Sharma01
Copy link
Author

Instead of a new model can you please put those fields on the project?

Sir, please check now.

DonnieBLT
DonnieBLT previously approved these changes Jan 24, 2025
View reviewed changes
@DonnieBLT
Copy link
Collaborator

Nice work, approved. Please check to see it merged.

@DonnieBLT
Copy link
Collaborator

Thank you

@DonnieBLT DonnieBLT enabled auto-merge (squash) January 24, 2025 08:16
auto-merge was automatically disabled January 24, 2025 08:23

Head branch was pushed to by a user without write access

@Dhruv-Sharma01
Copy link
Author

Thank you sir, just please review once more I have resolved issues arising due to pre-commit.

@DonnieBLT DonnieBLT enabled auto-merge (squash) January 24, 2025 14:50
auto-merge was automatically disabled January 24, 2025 17:06

Head branch was pushed to by a user without write access

@Dhruv-Sharma01
Copy link
Author

Sir, pre-commit was passing locally, I guess it was because of version mismatch I have fixed that please review once more.

@DonnieBLT DonnieBLT enabled auto-merge (squash) January 24, 2025 17:49
auto-merge was automatically disabled January 24, 2025 18:16

Head branch was pushed to by a user without write access

@Dhruv-Sharma01
Copy link
Author

Sir, error in tests is also resolved please review once more.

@DonnieBLT DonnieBLT enabled auto-merge (squash) January 24, 2025 18:20
@DonnieBLT
Copy link
Collaborator

can you please take out the myenv

DonnieBLT
DonnieBLT requested changes Jan 24, 2025
View reviewed changes
Copy link
Collaborator

@DonnieBLT DonnieBLT left a comment

Choose a reason for hiding this comment

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

myenv, otherwise looks good

auto-merge was automatically disabled January 24, 2025 19:08

Head branch was pushed to by a user without write access

@Dhruv-Sharma01
Copy link
Author

Yes sir, it is removed. I pushed it by mistake.

DonnieBLT
DonnieBLT requested changes Jan 26, 2025
View reviewed changes
website/migrations/0183_slackchannel.py

operations = [
migrations.CreateModel(
name="SlackChannel",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think slack channel coups link back to the project as some projects have multiple channels and can you squash the migrations to one file please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DonnieBLT DonnieBLT DonnieBLT requested changes

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.

Add a new field slack_channel to projects
4 participants
@Dhruv-Sharma01 @yashpandey06 @JisanAR03 @DonnieBLT