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

dynamic news additions #3149

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

dynamic news additions #3149

wants to merge 1 commit into from

Conversation

bbearce
Copy link
Contributor

@bbearce bbearce commented Apr 10, 2022

@ mention of reviewers

@Didayolo

A brief description of the purpose of the changes contained in this PR.

Add ability for addition of news through django admin panel

Known issues to be addressed in a separate PR

None

A checklist for hand testing

Any relevant files for testing

There is a live demo below and if you log in with provided user you can access the admin area to see the database.
Live demo: https://codalab-python3-test.eastus.cloudapp.azure.com/highlights

user: challenge-organizer; pass: testtest

Misc. comments

a) Old News posts that are currently on git are left as is statically on page. I did consider and started making a fixture to pre-load the database with (we can still do) , but I kind of thought where possible we should just have the functionality rather than data hard coded. Plus that news may not last forever and may not always be shown.

b) My migration addition for the NewsPost table also added alterations for the storage in the competitions table. I noticed past commits on git do this if migrations are added. One concern is that my Azure storage account key is checked in with the code in the migration file. This is a testing environment so it doesn't matter to me as I will delete soon anyways. However I'm wondering if this means I did something wrong or not. Anyways I think it's fine.

Checklist

  • Code review by me
  • Hand tested by me
  • I'm proud of my work
  • Code review by reviewer
  • Hand tested by reviewer
  • Ready to merge

Copy link
Collaborator

@Tthomas63 Tthomas63 left a comment

Choose a reason for hiding this comment

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

Minor nitpicks, nothing blocking except the migration story which is on my plate.

@@ -81,3 +81,5 @@ class PageAdmin(admin.ModelAdmin):
admin.site.register(models.CompetitionSubmission)
admin.site.register(models.CompetitionSubmissionMetadata)
admin.site.register(models.CompetitionDefBundle)

admin.site.register(models.NewsPost)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: No newline at EOF

migrations.AlterField(
model_name='team',
name='image',
field=models.FileField(blank=True, null=True, storage=codalab.azure_storage.AzureStorage(account_key='qY4fuziQqP0N63JurVsEvp7rioAsqnwPdRRgpLKDoCTNrVj1Xwt0l/EJVP3Yxn2IySXqjgMFYnvWd+B1mgPUAA==', account_name='codalabpython3test', azure_container='public'), upload_to='team_logo', verbose_name='Logo'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

On my side I'd like to look into the deconstruct method/decorator a bit to see if we can solve this before-hand.

return self.title

def __str__(self):
return self.title
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: No newline at EOF

@@ -143,15 +143,15 @@ class Highlights(TemplateView):
def get_context_data(self, *args, **kwargs):
context = super().get_context_data(*args, **kwargs)

data = Competition.objects.aggregate(
comp = Competition.objects.aggregate(
Copy link
Collaborator

@Tthomas63 Tthomas63 Apr 14, 2022

Choose a reason for hiding this comment

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

Nit: comp -> comps (Multiple objects vs one)

(This may even not be competition objects but raw data, the reasoning for the orig var name)

try:
news = NewsPost.objects.all()
for news_component in news:
nc = {'title':news_component.title,'link':news_component.link,'date':news_component.date,'post':news_component.post}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: More descriptive var name:
news_component

Flake8/PEP8 formatting:

news_component = {
    'title': news_component.title,
    'link': news_component.link,
    'date': news_component.date,
    'post': news_component.post
}

@@ -38,7 +41,7 @@
def clean_name(name):
return os.path.normpath(name).replace("\\", "/")


@deconstructible
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -2722,3 +2722,21 @@ def filename(self):
def competitiondump_post_delete_handler(sender, **kwargs):
comp_dump = kwargs['instance']
delete_key_from_storage(comp_dump, 'data_file')

class NewsPost(models.Model):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would the newsletter app be a better fit than web if the newsletter app is not niche

@Didayolo
Copy link
Member

@bbearce Was Tyler's review taken into account? Do you think this is ready to merge?

@bbearce
Copy link
Contributor Author

bbearce commented Oct 16, 2022

If I'm being honest, not yet. This would need a tiny bit of attention I think. Something about the @deconstructible decorator needs to be addressed. There is also a question about using the newsletter app versus the web app. I kind of dropped the ball on this when switching to codabench. I can revisit.

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