Skip to content

Forum quickreply templatization #5414

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

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

Conversation

rica-carv
Copy link
Contributor

Motivation and Context

Enable custom template for the forum quick reply form

Description

Changed code to take hardcoded HTML from forum quickreply shortcode to template

How Has This Been Tested?

Latest e107 github code

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist

Copy link

Code Climate has analyzed commit 90ce61e and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Bug Risk 1

The test coverage on the diff in this pull request is 7.1% (80% is the threshold).

This pull request will bring the total coverage in the repository to 39.0% (0.0% change).

View more on Code Climate.

@CaMer0n
Copy link
Member

CaMer0n commented Jan 29, 2025

@rica-carv Thank you for this! I left some comments. Just some minor tweaks and then I can commit it. Thank you.
Basically the form fields should all be shortcodes (other than 'form'). In other words, a bad template should not be able to break the functionality of the form.

@rica-carv
Copy link
Contributor Author

I left some comments.

@CaMer0n Sorry, but i don't see any coments.... anywhere....

@CaMer0n CaMer0n self-requested a review January 29, 2025 20:10
@CaMer0n
Copy link
Member

CaMer0n commented Jan 29, 2025

@rica-carv Sorry, It appears they went into a 'pending' state. Do you see them now?

@rica-carv
Copy link
Contributor Author

Do you see them now?

@CaMer0n Yes i do. Tx

@rica-carv
Copy link
Contributor Author

Done.
Added default template for legacy purposes, althought it's akward to remove embbebed HTML asd then left it there for legacy purposes....

@CaMer0n
Copy link
Member

CaMer0n commented Jan 30, 2025

Thanks. It's definitely better. Unfortunately, all the unit tests failed. Adding a defset('e_TOKEN') on e_TOKEN should fix it.
I added some more notes. We want to separate functionality from design, so that a theme developer can't break the functionality of the form.


  1. e_parse_shortcodeTest: Forum view shortcodes
    Test tests/unit/e_parse_shortcodeTest.php:testForumViewShortcodes

Error: ;1m [Error] Undefined constant "e_TOKEN"

#1 /__w/e107/e107/e107_plugins/forum/shortcodes/batch/view_shortcodes.php:1411
#2 /__w/e107/e107/e107_handlers/shortcode_handler.php:1154
#3 e_parse_shortcode->doCode
#4 /__w/e107/e107/e107_handlers/shortcode_handler.php:1020
#5 /__w/e107/e107/e107_handlers/e_parse_class.php:854
#6 /__w/e107/e107/e107_tests/tests/unit/e_parse_shortcodeTest.php:1717
#7 /__w/e107/e107/e107_tests/tests/unit/e_parse_shortcodeTest.php:1367
#8 /__w/e107/e107/e107_tests/vendor/bin/codecept:119

@rica-carv
Copy link
Contributor Author

rica-carv commented Jan 30, 2025

@CaMer0n I've got some more pulls regarding moving HTML to templates, if you have the time to have a look...

@rica-carv
Copy link
Contributor Author

@CaMer0n Think got it all donne... can you review it, please?

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