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

organisation api completed #629

Merged
merged 12 commits into from
Mar 17, 2024

Conversation

bharath637462
Copy link
Contributor

@bharath637462 bharath637462 commented Dec 26, 2023

Contributor checklist


Description

Related issue

Copy link

netlify bot commented Dec 26, 2023

Deploy Preview for activist-org canceled.

Name Link
🔨 Latest commit 0a5887a
🔍 Latest deploy log https://app.netlify.com/sites/activist-org/deploys/65f709b89fa6ee0008ce07e1

Copy link
Contributor

github-actions bot commented Dec 26, 2023

Thank you for the pull request!

The activist team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. It'd be great to have you!

Maintainer checklist

  • The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution

    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local activist repo
  • The TypeScript and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@andrewtavis
Copy link
Member

andrewtavis commented Dec 26, 2023

@bharath637462 we need to you to follow directions on what the work you're supposed to be doing is. #627 is assigned to me. You wrote into the issue asking if you could work on it, and you were not given permission to work on it. Please do not do any other backend API work at this time, and please focus on completing the work for #611. Once that is done you can begin working on #628 or another API issue once we have assigned it to you.

@to-sta and I will review this, but understand that further PRs like this where you haven't been assigned will be closed.

@andrewtavis
Copy link
Member

Do not send along further commits to this issue. We'll finalize it ourselves.

@andrewtavis andrewtavis requested review from andrewtavis and to-sta and removed request for andrewtavis December 26, 2023 18:27
@to-sta
Copy link
Collaborator

to-sta commented Dec 27, 2023

@andrewtavis

Workflows:
the pr_ci_backend check fail do to a simple linting issue and the mypy check fails because we are overwritting the ViewSet methods with a subclass of the Superclass.

Here is an example:

backend/entities/views.py:48: note:      Superclass:
backend/entities/views.py:48: note:          def create(self, request: Request, *args: Any, **kwargs: Any) -> Response
backend/entities/views.py:48: note:      Subclass:
backend/entities/views.py:48: note:          def create(self, request: Request) -> Response

Since we do not use *args and **kwargs we are overwritting the Superclass. I would say we ignore that issue and add a top level comment again with:

# mypy: disable-error-code="override"

API:
Since we need to have an Organization instance before we can create a organization_application, I would suggest to add creating the organization_application within the OrganizationViewSet's create method or overriding the Organization model save method.

The workflow for creating a Organization would be:

https://activist.org/de/organizations/create/ -> this would use the OrganizationViewSet create method (POST) to create an Organization if the user is authenticated. If it is successfully created, we would also create a organization_application instance with the default status. Here we need to have the organization_application_status before we can create an organization_application. We can create a fixture for this, that we load in with docker-compose.yaml.

Should we add a unqiue constrant to the name field of Organization model?

Other then that the create method looks fine to me!

@andrewtavis
Copy link
Member

I'll check this out a bit later, @to-sta. Wanted to remove the frontend changes so we're fresh :) Thanks for the details you've sent along!

@to-sta
Copy link
Collaborator

to-sta commented Feb 10, 2024

@andrewtavis I added the models for status as discussed. I added unique=True to the org. name. I was testing the endpoints with swagger ui and saw that i am able to create duplicates.

I made some changes to the serializer, now the deletion_date will be excluded from responses. We can get back to this when we work on the destroy method. created_by is set on the created method by the authenticated user, therefore the field should be a read_only field. status_updated and acceptance_date that should be set in the backend and therefore are also read_only.

class OrganizationSerializer(serializers.ModelSerializer[Organization]):
    class Meta:
        model = Organization
        exclude = ["deletion_date"]
        extra_kwargs = {
            "created_by": {"read_only": True},
            "social_accounts": {"required": False},
            "status_updated": {"read_only": True},
            "acceptance_date": {"read_only": True},
        }

@to-sta
Copy link
Collaborator

to-sta commented Feb 10, 2024

I additionally removed the logic from update, partial_update and destroy, since these methods still need to be properly implemented. Since these 3 methods only have pass instead of a return value the ci_backend workflow is failing.

@to-sta
Copy link
Collaborator

to-sta commented Feb 10, 2024

Thinking about the destroy method of the org API. We have a deletion_date field on the organization model. What logic should I implement here:

Keep the org after deletion (any fields that should be removed?), set a deletion_date (and status_updated) as well as assign a new status?

@andrewtavis
Copy link
Member

andrewtavis commented Mar 17, 2024

@to-sta, sorry for the broken backend run. There were a couple merge conflicts that needed to get figured out (rushed it a bit and just used GitHub for them as they seemed to be just accounting for some additions from other commits). I don't think that any of the changes that came through in this would be problematic, but maybe it's because there are parts of the code in here now that aren't accounted for in this PR as it's coming from other commits? Can you do a quick fix of the backend errors and I'll bring this in?

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

Thank you for all the hard work here, @to-sta! (also for the quick fixes just now 😅) So happy to have this finalized 🚀

@andrewtavis andrewtavis merged commit 106d8ec into activist-org:main Mar 17, 2024
6 checks passed
@andrewtavis andrewtavis mentioned this pull request Mar 17, 2024
2 tasks
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.

3 participants