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

Add external links to Chapter Profile #37

Merged
merged 21 commits into from
Nov 11, 2021

Conversation

Upasanadhameliya
Copy link
Contributor

@Upasanadhameliya Upasanadhameliya commented Oct 26, 2021

What?

Backend

  • Created a base model called Groups.
  • Made Chapters a child model class of Groups
  • Added Many-To-Many relationship in Groups with members and users. Many users can be a part of a group and a user can be involved in multiple groups.
  • Chapters objects were added to context after ordering by region and name. This was essential for the correct display through regroup template tag.
  • Added functionality for users to join and leave a Group.
  • Added GroupsIndexPage from which ChaptersIndexPage inherits.

Frontend

  • Added Chapters Index Page link to the Home Page
  • Added Chapters Page containing chapter title, Join/leave button and link to previous page.

Chapters Display on Chapters Index Page

ques71

The common fields such as "introduction line" and "content panel"
was added into the groups base class and the specific fields
were kept in the child class.
@Upasanadhameliya
Copy link
Contributor Author

@brylie I have made some initial commits in the favour of our discussion. I will be adding more changes in the same way. You can review my changes for any corrections/modifications as required.

project/chapters/models.py Outdated Show resolved Hide resolved
@brylie
Copy link
Collaborator

brylie commented Oct 26, 2021

Looks good so far! I only suggested a minor modification.

Introduction and Details fields were added to the Groups
page as all pages inheriting from groups would be requiring these
fields.
project/chapters/models.py Outdated Show resolved Hide resolved
@Upasanadhameliya
Copy link
Contributor Author

@brylie I am running into a problem while applying migrations. The following types of errors are being encountered:

Error1
You are trying to add a non-nullable field 'body' to chapter without a default; we can't do that (the database needs something to populate existing rows).
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows with a null value for this column)
 2) Quit, and let me add a default in models.py
Error2
You are trying to add a non-nullable field 'content' to homepage without a default; we can't do that (the database needs something to populate existing rows).
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows with a null value for this column)
 2) Quit, and let me add a default in models.py
Error3
You are trying to add a non-nullable field 'groups_ptr' to chapter without a default; we can't do that (the database needs something to populate existing rows).
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows with a null value for this column)
 2) Quit, and let me add a default in models.py

The error that I am encountering might be referenced in this issue

@brylie
Copy link
Collaborator

brylie commented Oct 26, 2021

Delete your existing db.sqlite and then run python manage.py makemigrations. Note, you can set the body and content fields to be nullable with null=True and blank=True.

1. Single introduction field was added of the type
RichText field.

2. The "content" field in the home/models.py/ : HomePage
was made to be optional due to the default value restriction
during migrations.

3. Reformatted the home and chapters models using black.
@Upasanadhameliya
Copy link
Contributor Author

Hey @brylie I have been brainstorming about our issue and I tried to create some test data for the same. So currently the countries are getting rendered with their regions in separate boxes. But won't you want to list all the countries of the same region together? Like in the following example won't you want japan to be under india?
ques61
Forgive my basic illustration done with paint 😆

@brylie
Copy link
Collaborator

brylie commented Oct 28, 2021

Yes, we want to group chapters by region. Since grouping data by an attribute is such a common need, Django provides the regrouo template tag. Let's regroup the chapters by region. Pick suitable Bootstrap components to display the regrouped chapters.

A limitation of {% regroup %} was it's inability to order
the groups before grouping and thus had to be grouped explicitly.

Also added links to return to the parent pages from the child pages.
The many-to-many relationship was used to add or remove
a particular user to the selected chapter. Now a single
user can join one or more chapters and a single chapter
can contain one or more users as well.
@Upasanadhameliya
Copy link
Contributor Author

@brylie I have implemented the user membership feature in the chapters. You might want to check it out. Currently I am using query string parameter on the button to achieve this, but there might be a better way to do it. I might even consider using ajax to prevent page refresh. Feel free to give some suggestions for the same!

@brylie
Copy link
Collaborator

brylie commented Oct 29, 2021

I might even consider using ajax to prevent page refresh.

For dynamic portions of the page that should update without a refresh, let's use HTMX. We basically need to define a view that returns the HTML fragment for the user membership component. When the user clicks the button, HTMX will make a request to the membership widget view and insert the returned HTML fragment into the template.

@brylie
Copy link
Collaborator

brylie commented Oct 29, 2021

However, we might want to live with the page refresh for this iteration unless AJAX/HTMX seems reasonable to add in the first pass.

@Upasanadhameliya
Copy link
Contributor Author

@brylie so should I make this pull request ready for review? Is there any additional change you would like me to implement before I do so?

@brylie
Copy link
Collaborator

brylie commented Nov 1, 2021

Yeah, let's mark this as ready for review. If you agree that the AJAX would be a good enhancement for the group membership button, please open a feature request, so we remember to come back around to that enhancement.

@Upasanadhameliya Upasanadhameliya marked this pull request as ready for review November 2, 2021 05:08
project/chapters/models.py Outdated Show resolved Hide resolved
project/chapters/models.py Show resolved Hide resolved
It was a bad practice for updating the database using a get method,
which was the case with chapter membership. The new method
implemented uses a POST method, along with form validation to ensure
site safety and security and preventing url/sql injection or other
hacking techniques.
project/chapters/templates/chapters/chapter.html Outdated Show resolved Hide resolved
project/chapters/templates/chapters/chapter.html Outdated Show resolved Hide resolved
project/chapters/templates/chapters/chapter.html Outdated Show resolved Hide resolved
ChaptersIndexPage was made to inherit from the abstract Page
model so that the code becomes easier to maintain. Other types
of Groups and their Index Pages would be created easily by just
inheriting from Groups and GroupsIndexPage.
project/chapters/models.py Outdated Show resolved Hide resolved
project/chapters/models.py Outdated Show resolved Hide resolved
project/chapters/models.py Outdated Show resolved Hide resolved
project/chapters/models.py Outdated Show resolved Hide resolved
project/chapters/models.py Outdated Show resolved Hide resolved
@brylie
Copy link
Collaborator

brylie commented Nov 10, 2021

The overall direction and code architecture here look great! There are just a few minor considerations that I've mentioned in the review comments.

Instead of specifying a hardcoded class "Chapter", the subpage class
was derived from the object and the child objects were fetched
and ordered accordingly.

The models were reformatted with black.
It wasn't preferrable to redirect the unauthenticated users to login
page and it was best to not render the join group button to
non authenticated users.
@brylie brylie merged commit 8e85e78 into Wagify:develop Nov 11, 2021
@brylie
Copy link
Collaborator

brylie commented Nov 11, 2021

Great work! This is a significant PR :-)

@Upasanadhameliya
Copy link
Contributor Author

Yeah thank you! 😄

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.

2 participants