Feature/lore wiki page #675

Gunvor4 · 2023-09-08T15:07:55Z

Proposed changes

Added a MAKE lore wiki to the internal page, so members can write wiki articles about anything lore in the MAKE universe.

Areas to review closely

Deployment notes

After deployment, permissions for the general MAKE group should be changed, so every member of MAKE is able to both view, add, change and delete lore articles.

Checklist

(If any of the points are not relevant, mark them as checked)

Gunvor4 · 2023-09-08T15:09:13Z

codecov · 2023-09-08T15:12:23Z

Codecov Report

Merging #675 (4d9e2a4) into dev (6f55b7d) will decrease coverage by 0.55%.
The diff coverage is 56.60%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #675      +/-   ##
==========================================
- Coverage   88.16%   87.62%   -0.55%     
==========================================
  Files         152      152              
  Lines        6187     6288     +101     
==========================================
+ Hits         5455     5510      +55     
- Misses        732      778      +46

Files	Coverage Δ
src/internal/admin.py	`100.00% <100.00%> (ø)`
src/internal/urls.py	`100.00% <100.00%> (ø)`
src/internal/models.py	`90.39% <58.82%> (-3.40%)`	⬇️
src/internal/views.py	`85.14% <64.15%> (-5.77%)`	⬇️
src/internal/forms.py	`75.18% <31.03%> (-11.98%)`	⬇️

elisakiv · 2023-09-28T18:27:14Z

It looks great, just few comments:

It seems that when the title of an article is one very long word with no spaces, it behaves strangely on mobile. The name does not wrap when it becomes too long so the title continues on one line, past the length of the header.

When the title is long, but does contain spaces, the placement of the edit and delete button is a little awkward.

elisakiv · 2023-09-28T18:28:29Z

If a users adds an article with a name only consiting of special character, the page crashes.

elisakiv · 2023-09-28T18:28:39Z

When editing an article with a picture, the button and checkbox for removing the picture behaves a little strangely. The checkbox is not level with the picure name, and the "Clear" button does not fully behave like a button, mainly because the cursor does not become a hand, the user does not receive any feedback that the button was clicked, and it does not look like a button either, it looks like the other text on the page.

elisakiv · 2023-09-28T19:00:34Z

If the article has a very long name, the title in the menu overlaps with the text of the article that the user has open

Created Lore model, modified save() method to always slugify title, set up compression of images, and added new model to Django admin

Made form to be used when adding or changing lore articles

Added URLs and views for viewing the general lore article page and for viewing a particular lore article, for adding new lore article, and changing and deleting existing articles. Modified form_valid() method for the CreateView and the UpdateView, so the page throws an error if a user tries to write a new article on a topic that already exists. Added new template for viewing lore articles, and added lore wiki to the internal header.

Updated translation files before rebasing

If the name of the lore article is "Dev", it should not be editable and not deletable, so we can tell everyone at MAKE that Dev is the best committee, and there is nothing they can do to change that. Mohaha.

Updated translation files before rebasing

Resolved merge conflicts in changelog

Made changes to burger menu, so that it changes into a cross when open

Added separate CSS code for views on pads and mobile phones

Everyone at MAKE should be able to both view, add, change and delete lore articles. This must be added to permissions for the general MAKE group.

…creens Changed how the icons for changing and deleting lores are shown on devices with smaller screens. On computers, they are shown floating right of the title of the lore article. On pads and mobile phones, the icons are now shown below the title.

Changed text wrapping of long words, switched from visibility: hidden to display: none to make placement of elements more logical, moved pencil and trash can icon to its own line both for normal screen and for mobile view, changed bottom-paddings.

Commented on checkbox in template, in case future dev members need an explanation

Set max length of main text field of lore articles to 150000 letters, and ran migrations

Added an error to deal with titles only consisting of symbols. Checking if title can be slugified and if it results in a non-empty text string. If not, the error is shown.

Resoved merge conflicts in .po and .mo files

When a title contains an 'ø' or an 'æ', they seem to be left out when the title is slugified. I now fixed this by replacing 'ø' with 'o' and 'æ' with 'ae' before slugifying the title

ddabble

Very nice job! 🤩 Just a few comments 😅

I guess I could comment more on the CSS, but I can potentially wait with that until after you've made some of the changes I suggested below - if you decide to 🙂 (Otherwise, I must admit that I personally don't find CSS code in general to be particularly important to keep polished :))

Finally, it would also be very nice if you could add some (simple) tests for the new code, so that we can feel more secure that it keeps working the way we think it does - both now and after future changes to the codebase 🙂

src/internal/migrations/0028_alter_lore_text.py

src/internal/admin.py

src/internal/models.py

src/internal/templates/internal/lore_wiki.html

src/internal/static/internal/css/make_lore.css

Also removed image from model, form and template, since images can now be uploaded by CKEditor. Added javascript file to adjust margins of images uploaded by CKEditor

Renamed files: lore_wiki.html -> lore_list.html lore_articles_list.html -> lore_list__article_links.html make_lore.css -> lore_list.css Changed placement of html files. Instead of keeping the file lore_list__article_links.html in a separate file called includes, now both this file and lore_list.html has been moved to a folder called lore.

Removed overwritten form_valid methods from views and instead overwriting the clean method in forms.

Changed names of urls in urls.py and views.py: lore_article -> lore_detail update_lore -> lore_update delete_lore -> lore_delete add_lore -> lore_create

Deleted multiple migration files, and reran makemigrations, so the lore model now only have on migration file

Removed the overwritten get_context_data method, and instead using extra_context

The name previously used for the menu - 'lore_topics' has now been changed to 'all_lore_articles', while the name used for the lore currently being shown - 'lore_article' has been changed to 'shown_lore_article'

models.py: Altered max_length on the title field, made the slug unique, altered __str__ method, made sure titles start with a capitalized letter, added verbose_name. views.py: Added blank lines where requested, changed form_title and back_button_text for LoreFormMixin, LoreCreateView and LoreUpdateView.

src/internal/admin.py

ddabble

Just doing a second round after the most recent commits 🙂

I just remembered that I might not have taught you anything about writing tests..? If so, just send a DM to me (or maybe @elisakiv..?) if you'd like some pointers :)

In any case, I wanted to reiterate that it would be great if you could add some (simple) tests for the lore pages, which would both help us feel more secure that the code behaves as we think it does - especially in edge cases - and help future developers understand the intended (main) functionality of the code by showing some typical scenarios in some of the tests - as tests often do. Examples include:

That a MAKE member (i.e. a user with some of the lore permissions) is able to view/add/change(/delete?) lore pages (through GET and/or POST requests), but an unprivileged user is not
That the slug field has the expected value after submitting various titles
That colliding slugs/titles are correctly identified
That the slug/title field cannot be empty
Anything else that you think would be useful to have tests for 🙂

src/internal/forms.py

src/internal/models.py

src/internal/views.py

src/internal/forms.py

src/internal/templates/internal/lore/lore_list.html

ddabble · 2023-12-15T22:38:03Z

src/internal/views.py

+        title = str(self.object)
+        title = title.replace('ø', 'o')
+        title = title.replace('æ', 'ae')
+        slug = slugify(title)
+        return reverse_lazy('lore_detail', args=[slug])


As mentioned in a comment above, this (and the same method of LoreUpdateView) can be changed to the following after adding get_absolute_url() to Lore:

Suggested change

title = str(self.object)

title = title.replace('ø', 'o')

title = title.replace('æ', 'ae')

slug = slugify(title)

return reverse_lazy('lore_detail', args=[slug])

return self.object.get_absolute_url()

Also, since both the mentioned views' implementations of get_success_url() are completely identical, they can be removed and instead moved to LoreFormMixin 🙂

ddabble · 2023-12-15T22:43:19Z

src/internal/views.py

+        title = str(self.get_form_kwargs()['instance'])
+        title = title.replace('ø', 'o')
+        title = title.replace('æ', 'ae')
+        slug = slugify(title)
+        return reverse_lazy('lore_detail', args=[slug])


This should be possible to replace with:

Suggested change

title = str(self.get_form_kwargs()['instance'])

title = title.replace('ø', 'o')

title = title.replace('æ', 'ae')

slug = slugify(title)

return reverse_lazy('lore_detail', args=[slug])

return self.get_success_url()

ddabble · 2023-12-15T22:43:53Z

src/internal/views.py

+    permission_required = ('internal.change_lore',)
+
+    form_title = _("Change Lore Article")
+    back_button_text = _("Lore Article")


Perhaps it would be nice to have the back button simply say the title of the article you're editing? 🤔 What do you think?

Suggested change

back_button_text = _("Lore Article")

def get_back_button_text(self):

return self.object.title

ddabble · 2023-12-15T22:45:10Z

src/web/static/web/css/style.css

@@ -209,7 +209,6 @@ span.no-wrap {
    z-index: -1;
 }

-


This change can be reset :)

ddabble · 2023-12-15T23:27:30Z

Btw, regarding the deployment notes in the PR description, it might be wise to not let all members of MAKE delete lore articles, and instead only grant that permission to e.g. the board members - similar to the group permissions for the internal quotes currently on the website 🤔 What do you think?

src/internal/models.py

Gunvor4 added the feature New functionality on the page label Sep 8, 2023

Gunvor4 requested review from ddabble and elisakiv September 8, 2023 15:07

Gunvor4 self-assigned this Sep 8, 2023

Gunvor4 changed the base branch from main to dev October 5, 2023 18:04

Gunvor4 requested a review from VebjornNyvoll November 2, 2023 19:45

Gunvor4 added 18 commits November 9, 2023 18:52

Created new model Lore

aa4ac4f

Created Lore model, modified save() method to always slugify title, set up compression of images, and added new model to Django admin

Made form for lore articles

17d65c5

Made form to be used when adding or changing lore articles

Added styling to lore wiki

1a3e879

Updated translation files

99080f8

Updated translation files before rebasing

Added internal joke

33d45af

If the name of the lore article is "Dev", it should not be editable and not deletable, so we can tell everyone at MAKE that Dev is the best committee, and there is nothing they can do to change that. Mohaha.

Added hamburger menu to be shown on smaller screens

64c9027

Updated translation files

dea71b2

Updated translation files before rebasing

Updated changelog

d5ee29d

Resolved merge conflicts in changelog

Adjusted burger menu

c0f69d7

Made changes to burger menu, so that it changes into a cross when open

Adjusted media rules

9f2d6e5

Added separate CSS code for views on pads and mobile phones

Changed permissions

49257d6

Everyone at MAKE should be able to both view, add, change and delete lore articles. This must be added to permissions for the general MAKE group.

Made CSS changes to lore page

31443bf

Changed text wrapping of long words, switched from visibility: hidden to display: none to make placement of elements more logical, moved pencil and trash can icon to its own line both for normal screen and for mobile view, changed bottom-paddings.

Commented on checkbox

da6d810

Commented on checkbox in template, in case future dev members need an explanation

Set max length on main text field for lore article

ff5aefb

Set max length of main text field of lore articles to 150000 letters, and ran migrations

Added error to deal with titles consisting of symbols

a99bb65

Added an error to deal with titles only consisting of symbols. Checking if title can be slugified and if it results in a non-empty text string. If not, the error is shown.

Changed the placement of the checkbox in the lore article update form

d8192ac

Gunvor4 added 2 commits November 9, 2023 19:00

Updated translation files

ba0a763

Resoved merge conflicts in .po and .mo files

Improved slugification of titles

6f6f595

When a title contains an 'ø' or an 'æ', they seem to be left out when the title is slugified. I now fixed this by replacing 'ø' with 'o' and 'æ' with 'ae' before slugifying the title

Gunvor4 force-pushed the feature/lore-wiki-page branch from 972bf22 to 6f6f595 Compare November 9, 2023 18:06

elisakiv approved these changes Nov 9, 2023

View reviewed changes

ddabble requested changes Nov 11, 2023

View reviewed changes

Gunvor4 added 11 commits November 18, 2023 23:43

Fixed alphabetical order of imports

f38a527

Changed content field on model from TextField to RichTextUploadingField

3bf1b3a

Also removed image from model, form and template, since images can now be uploaded by CKEditor. Added javascript file to adjust margins of images uploaded by CKEditor

Switched from validating form data in views to validating in forms

f1e3c35

Removed overwritten form_valid methods from views and instead overwriting the clean method in forms.

Changed url names

b16dc06

Changed names of urls in urls.py and views.py: lore_article -> lore_detail update_lore -> lore_update delete_lore -> lore_delete add_lore -> lore_create

Changed migrations

b89ea66

Deleted multiple migration files, and reran makemigrations, so the lore model now only have on migration file

Switched from overriding get_context_data to using extra_context

e2bfa3d

Removed the overwritten get_context_data method, and instead using extra_context

Changed context object names

2f5da3d

The name previously used for the menu - 'lore_topics' has now been changed to 'all_lore_articles', while the name used for the lore currently being shown - 'lore_article' has been changed to 'shown_lore_article'

Updated translation files

94e8144

Removed additional CSS code for update forms from style.css

4d9e2a4

ddabble mentioned this pull request Nov 26, 2023

Added page to show statistics from use of the workshop #669

Open

10 tasks

Gunvor4 requested a review from ddabble December 2, 2023 12:05

ddabble reviewed Dec 9, 2023

View reviewed changes

src/internal/admin.py Show resolved Hide resolved

ddabble requested changes Dec 15, 2023

View reviewed changes

ddabble reviewed Jan 23, 2024

View reviewed changes

src/internal/models.py Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/lore wiki page #675

Feature/lore wiki page #675

Gunvor4 commented Sep 8, 2023

Gunvor4 commented Sep 8, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

elisakiv commented Sep 28, 2023 •

edited

Loading

elisakiv commented Sep 28, 2023

elisakiv commented Sep 28, 2023

elisakiv commented Sep 28, 2023

ddabble left a comment •

edited

Loading

ddabble left a comment

ddabble Dec 15, 2023

ddabble Dec 15, 2023

ddabble Dec 15, 2023

ddabble Dec 15, 2023

ddabble commented Dec 15, 2023

-    back_button_text = _("Lore Article")
+    def get_back_button_text(self):
+        return self.object.title

Feature/lore wiki page #675

Are you sure you want to change the base?

Feature/lore wiki page #675

Conversation

Gunvor4 commented Sep 8, 2023

Proposed changes

Areas to review closely

Deployment notes

Checklist

Gunvor4 commented Sep 8, 2023

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

elisakiv commented Sep 28, 2023 • edited Loading

elisakiv commented Sep 28, 2023

elisakiv commented Sep 28, 2023

elisakiv commented Sep 28, 2023

ddabble left a comment • edited Loading

Choose a reason for hiding this comment

ddabble left a comment

Choose a reason for hiding this comment

ddabble Dec 15, 2023

Choose a reason for hiding this comment

ddabble Dec 15, 2023

Choose a reason for hiding this comment

ddabble Dec 15, 2023

Choose a reason for hiding this comment

ddabble Dec 15, 2023

Choose a reason for hiding this comment

ddabble commented Dec 15, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

elisakiv commented Sep 28, 2023 •

edited

Loading

ddabble left a comment •

edited

Loading