Skip to content

Renaming category #555

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

Merged
merged 19 commits into from
Feb 4, 2021
Merged

Renaming category #555

merged 19 commits into from
Feb 4, 2021

Conversation

seyfeb
Copy link
Collaborator

@seyfeb seyfeb commented Jan 23, 2021

  • added API endpoint ('url' => '/api/category/{category}', 'verb' => 'PUT')
  • made categories in navigation pane editable

Closes #322

@codecov
Copy link

codecov bot commented Jan 23, 2021

Codecov Report

Merging #555 (74f7608) into master (b42b21e) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #555      +/-   ##
===========================================
- Coverage      1.02%   1.01%   -0.02%     
- Complexity      434     440       +6     
===========================================
  Files            13      13              
  Lines          1366    1381      +15     
===========================================
  Hits             14      14              
- Misses         1352    1367      +15     
Flag Coverage Δ Complexity Δ
integration 0.00% <0.00%> (ø) 0.00 <6.00> (ø)
unittests 1.01% <0.00%> (-0.02%) 0.00 <6.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
lib/Controller/MainController.php 0.00% <0.00%> (ø) 39.00 <6.00> (+6.00)

@seyfeb seyfeb force-pushed the feature/issue332renameCat branch from ac158bb to 258302e Compare January 23, 2021 22:05
@seyfeb seyfeb requested a review from christianlupus January 23, 2021 22:05
@seyfeb seyfeb marked this pull request as draft January 24, 2021 11:23
@seyfeb seyfeb added Backend Issue or PR related to the backend code enhancement New feature or request Frontend Issue or PR related to the frontend code javascript Pull requests that update Javascript code php Pull requests that update Php code labels Jan 24, 2021
Copy link
Collaborator

@christianlupus christianlupus left a comment

Choose a reason for hiding this comment

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

I found an issue with the code changes when you change the name multiple times. Do you want to fix this before merging or want me to open a new issue afterward to keep things separated?

@christianlupus
Copy link
Collaborator

One more thing: ATM I have to reject the PR. We need to update the API minor version as we have some changes of the API endpoints.

@seyfeb
Copy link
Collaborator Author

seyfeb commented Jan 24, 2021

I found an issue with the code changes when you change the name multiple times. Do you want to fix this before merging or want me to open a new issue afterward to keep things separated?

I did quite some changes since your comment. @christianlupus Does this still exist?

One more thing: ATM I have to reject the PR. We need to update the API minor version as we have some changes of the API endpoints.

Okay, we could also create a separate controller for categories.

@seyfeb seyfeb force-pushed the feature/issue332renameCat branch from 30d4ac7 to 8559ae7 Compare January 24, 2021 20:11
@seyfeb seyfeb marked this pull request as ready for review January 24, 2021 20:40
@christianlupus
Copy link
Collaborator

I found an issue with the code changes when you change the name multiple times. Do you want to fix this before merging or want me to open a new issue afterward to keep things separated?

I did quite some changes since your comment. @christianlupus Does this still exist?

Not exactly the same but similar. As an image says more than 1000 words and a video has 25 images per sec, this is worth a whole book 😄:

vokoscreenNG-2021-01-25_08-47-30.mp4

One more thing: ATM I have to reject the PR. We need to update the API minor version as we have some changes of the API endpoints.

Okay, we could also create a separate controller for categories.

No, keeping that in the main controller is OK so far (no talked from a codebase refactoring perspective).
What I mean is the fact that 3rd part apps like the one from @Teifun2 use our API endpoints as REST service. To allow them to have a clear understanding, which cookbook app version is running, I introduced #487.
The cookbook version is incremented automatically by the bumping script but the API version must be handled manually. I just pushed a commit to update to API version 0.2.

@seyfeb
Copy link
Collaborator Author

seyfeb commented Jan 25, 2021

Alright, this was really helpful. After updating some more stuff, I think this problem should be fixed and I was able to do some other improvements as well.

@GutHib
Copy link

GutHib commented Jan 25, 2021

Seems like you guys can read my thoughts. Nice addition - thank you.

seyfeb added 13 commits February 4, 2021 16:16
Signed-off-by: Sebastian Fey <[email protected]>
…ory page if category is currently opened

Signed-off-by: Sebastian Fey <[email protected]>
Signed-off-by: Sebastian Fey <[email protected]>
seyfeb and others added 5 commits February 4, 2021 16:18
…itor when recipe's category is updated

Signed-off-by: Sebastian Fey <[email protected]>
…ed; don't reload complete recipe data anymore

Signed-off-by: Sebastian Fey <[email protected]>
…ate of the recipe in store if its category was renamed.

Signed-off-by: Sebastian Fey <[email protected]>
@christianlupus
Copy link
Collaborator

I just had a look and it seems to work "mostly". I have one issue but it seems this is an upstream bug of the nextcloud-vue components: Open the edit field and try to move the cursor by clicking with the mouse. In my instance, this triggered the :to link to be navigated. If @seyfeb has no idea why this is happening, I think we can merge this PR. I will just rebase to get rid of the conflicts.

@christianlupus christianlupus force-pushed the feature/issue332renameCat branch from 9c8d490 to 70cc940 Compare February 4, 2021 15:19
@seyfeb
Copy link
Collaborator Author

seyfeb commented Feb 4, 2021

I just had a look and it seems to work "mostly". I have one issue but it seems this is an upstream bug of the nextcloud-vue components: Open the edit field and try to move the cursor by clicking with the mouse. In my instance, this triggered the :to link to be navigated. If @seyfeb has no idea why this is happening, I think we can merge this PR. I will just rebase to get rid of the conflicts.

We might want to consider updating the @nextcloud/vue dependency and see if this issue is gone. However this is quite a version jump so we need to carefully check the breaking changes of the versions in between.

@christianlupus
Copy link
Collaborator

I am going to merge this now and open an issue that might get closed once the problem is identified and solved.

@christianlupus christianlupus merged commit 6b8cdf0 into master Feb 4, 2021
@delete-merged-branch delete-merged-branch bot deleted the feature/issue332renameCat branch February 4, 2021 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend Issue or PR related to the backend code enhancement New feature or request Frontend Issue or PR related to the frontend code javascript Pull requests that update Javascript code php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch category renaming
3 participants