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

Feat/duplication #1180

Draft
wants to merge 27 commits into
base: doryphore-dev
Choose a base branch
from
Draft

Feat/duplication #1180

wants to merge 27 commits into from

Conversation

mrflos
Copy link
Contributor

@mrflos mrflos commented Jul 1, 2024

Description of pull request / Description de la demande d'ajout

90% finished feature of duplication of pages/lists in same wiki or to external wikis

After rebase from doryphore-dev i had some strange bugs, i tried to fix them, hopefully i got them all.

You can test the duplication feature on another wiki by giving url local dev url as url for duplication (it will use the api roads instead of the controller).

Sorry for the quite big codebase changes...

Edit: i can see what when wrong : there are two other beta-features included : an image optimizer, and extra-fields feature (that add comments and reactions to bazarlist if needed)
I will check if everything is working as expected, as it should also be reviewed, or do you prefer separated PRs ?

@mrflos mrflos requested a review from seballot July 1, 2024 10:29
@mrflos mrflos marked this pull request as draft July 1, 2024 10:30
javascripts/handlers/duplicate.js Fixed Show fixed Hide fixed
javascripts/handlers/duplicate.js Fixed Show fixed Hide fixed
@seballot seballot force-pushed the feat/duplication branch from a9c4a8b to 301df7f Compare July 1, 2024 12:13
Copy link
Contributor

@seballot seballot left a comment

Choose a reason for hiding this comment

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

WIP review, j'ai l'impression que github a perdu la moitié de mes commentaires..

-> edit c'est bon il les a gardé, je continue alors

templates/handlers/duplicate.twig Outdated Show resolved Hide resolved
handlers/DuplicateHandler.php Outdated Show resolved Hide resolved
handlers/DuplicateHandler.php Show resolved Hide resolved
includes/controllers/ApiController.php Show resolved Hide resolved
includes/controllers/ApiController.php Show resolved Hide resolved
includes/services/DuplicationManager.php Outdated Show resolved Hide resolved
includes/services/ImportFilesManager.php Outdated Show resolved Hide resolved
includes/services/PageManager.php Outdated Show resolved Hide resolved
}, $pages);

return $pages;
}

private function duplicate($sourceTag, $destinationTag): bool
Copy link
Contributor

Choose a reason for hiding this comment

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

?? un code de test? à supprimer non?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

non je voulais creer une methode dans le PageManager, mais work in progress

javascripts/handlers/duplicate.js Show resolved Hide resolved
Copy link
Contributor

@seballot seballot left a comment

Choose a reason for hiding this comment

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

Yo !

top tout ça !

J'ai fait des petites remarques pour la forme, mais c'est pas du tout bloquant. y'a 2/3 commentaires à fixer cependant

J'ai poussé les petit fix, plus un passage de linter (ils étaient pas en place quand tu as commencé le boulot sur cette branche)

templates/handlers/duplicate.twig Outdated Show resolved Hide resolved
@@ -1180,4 +1180,11 @@ function ($attributeName) {

return $entriesIds;
}

private function duplicate($sourceTag, $destinationTag): bool
Copy link
Contributor

Choose a reason for hiding this comment

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

pareil, test code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pareil methode de service non existante encore..

tools/bazar/actions/BazarListeAction.php Outdated Show resolved Hide resolved
@@ -36,6 +36,11 @@
</td>
<td>{{ key }}</td>
<td>
{% if list.canDuplicate %}
<a class="btn btn-default btn-xs" href="{{ url({ params: { vue: 'listes', action: 'duplicate', idliste: key }}) }}">
<i class="far fa-clone"></i>
Copy link
Contributor

Choose a reason for hiding this comment

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

On est d'accord c'est pas encore implémenté la duplication de liste? en tout cas chez moi ça marche pas :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oui ca vient

@mrflos
Copy link
Contributor Author

mrflos commented Jul 1, 2024

Ok merci pour cette revue, je vais tout prendre en compte, sauf peut etre la re--ecriture jquery vers vuejs, je ferai que si j'ai du temps.

@mrflos mrflos force-pushed the feat/duplication branch 2 times, most recently from 6715402 to 14b7d39 Compare July 31, 2024 18:39
javascripts/handlers/duplicate.js Fixed Show fixed Hide fixed
javascripts/handlers/duplicate.js Fixed Show fixed Hide fixed
@mrflos mrflos force-pushed the feat/duplication branch 2 times, most recently from b989037 to c38df35 Compare August 2, 2024 10:12
data: $('#form-duplication').serialize()
}).done((d) => {
if (btnAction === 'open') {
document.location = `${shortUrl}/?${d.newTag}`

Check warning

Code scanning / CodeQL

DOM text reinterpreted as HTML Medium

DOM text
is reinterpreted as HTML without escaping meta-characters.
if (btnAction === 'open') {
document.location = `${shortUrl}/?${d.newTag}`
} else if (btnAction === 'edit') {
document.location = `${shortUrl}/?${d.newTag}/edit`

Check warning

Code scanning / CodeQL

DOM text reinterpreted as HTML Medium

DOM text
is reinterpreted as HTML without escaping meta-characters.
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