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

Notice admin updated #225

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Notice admin updated #225

wants to merge 3 commits into from

Conversation

dhruvhacks
Copy link
Member

Following PR fixes #52 and fixes #170
Notices bookmarked by users are listed at end of that specific notice now.
Admin Interface
notice_admin

displaying bookmarked notice section at the end of edit notice menu
bookmarked_notice_view

@RishabhJain2018
Copy link
Member

@dhruvhacks Can you please resolve the conflicts.

@dhruvhacks
Copy link
Member Author

@RishabhJain2018 Merge conflicts resolved and build failures fixed. Please review the PR.

RishabhJain2018
RishabhJain2018 requested changes Sep 25, 2017
View reviewed changes
notices/admin.py Outdated


class NoticeAdmin(ImportExportModelAdmin):
Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't remove the ImportExportModelAdmin from Notice admin.

notices/admin.py Outdated


admin.site.register(Notice, NoticeAdmin)
admin.site.register(BookmarkedNotice, BookmarkedNoticeAdmin)
admin.site.register(TrendingInCollege, TrendingInCollegeAdmin)
# admin.site.register(TrendingInCollege, TrendingInCollegeAdmin) #Unused model
Copy link
Member

Choose a reason for hiding this comment

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

Well, this isn't the unused model. we display it on the landing page of Infoconnect. Please revert this.

wifi/models.py Outdated
@@ -5,40 +5,50 @@

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think your editor have screwed up the indentation. Can you please fix it.

wifi/views.py Outdated
@@ -17,116 +17,128 @@

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this file is a part of this PR. Can you please remove it.

wifi/models.py Outdated
@@ -5,40 +5,50 @@


Copy link
Member

Choose a reason for hiding this comment

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

I don't think this file is a part of this PR. Can you please remove it.

@dhruvhacks
Copy link
Member Author

@RishabhJain2018 All the requested changes have been reverted.
Please review the PR.

RishabhJain2018
RishabhJain2018 requested changes Sep 27, 2017
View reviewed changes
notices/admin.py Outdated
}),
)


class TrendingInCollegeAdmin(ImportExportModelAdmin):
class TrendingInCollegeAdmin(admin.ModelAdmin):
Copy link
Member

Choose a reason for hiding this comment

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

The admin class change isn't the part of this PR. We can have a separate PR for it. Can you please change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure sir. 🙂

@dhruvhacks
Copy link
Member Author

@RishabhJain2018 changes updated, Please review the PR. 🙂

@RishabhJain2018
Copy link
Member

RishabhJain2018 commented Oct 6, 2017 •
edited
Loading

Hey @dhruvhacks I checked it and found that it would not be a good way of displaying it, once the list populates with large number of users. Currently, we can add searching the Bookmarks admin.
Let me know your thoughts over it.

@RishabhJain2018
Copy link
Member

@dhruvhacks Any update on this??

@dhruvhacks
Copy link
Member Author

dhruvhacks commented May 15, 2018 •
edited
Loading

Currently, we can add searching the Bookmarks admin.
Let me know your thoughts over it.

@RishabhJain2018
Sir,
I felt that having separate admin display for bookmarked notices would be vague since all the notices and users will be mixed within one. This PR appends a section for Bookmarked notice within the notice admin since all the users will be grouped together under a specific notice and it would be easier to access.

I checked it and found that it would not be a good way of displaying it, once the list populates with large number of users.

We may set a limit (say 10) for no. of users to be displayed who bookmarked the specific notice.
Currently admin.TabularInline method does not provide any scope for adding search bar due to it's display layouts. Moreover in case of such InlineModelAdmin, possible filter methods include filter_vertical (link) but it also requires a ManyToMany field and notices.users is a ForeignKey field.

Sorry for the late reply.

@RishabhJain2018
Copy link
Member

@dhruvhacks Feel free to merge this as well.

@dhruvhacks
Copy link
Member Author

@RishabhJain2018 Okay sir. Will update the PR and consider merging!

@dhruvhacks dhruvhacks added the stale Needs re-work label Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RishabhJain2018 RishabhJain2018 RishabhJain2018 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
stale Needs re-work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show 100 notices on django admin on the Notice Page Improving Admin.py of every App
2 participants
@dhruvhacks @RishabhJain2018