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

Force disabling single-quote list for type parameter #2632

Merged
merged 3 commits into from
Mar 15, 2022
Merged

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Feb 4, 2022

Fix #1107


Debug session with @miaulalala

Impossible to override the stringify.propertyValue as we don't want to do that for all types,

Suggestion: override the toString method and use a regex to force-remove quotes*
/TYPE="([a-zA-Z-,]+)"/gm

@skjnldsv skjnldsv added 2. developing Work in progress bug Something isn't working technical debt labels Feb 4, 2022
@codecov
Copy link

codecov bot commented Feb 4, 2022

Codecov Report

Merging #2632 (b34df2d) into main (67a9da3) will decrease coverage by 50.80%.
The diff coverage is 94.44%.

@@              Coverage Diff              @@
##               main    #2632       +/-   ##
=============================================
- Coverage     68.85%   18.04%   -50.81%     
  Complexity      248      248               
=============================================
  Files            21       97       +76     
  Lines           700     3169     +2469     
  Branches          0      439      +439     
=============================================
+ Hits            482      572       +90     
- Misses          218     2404     +2186     
- Partials          0      193      +193     
Impacted Files Coverage Δ
...c/components/AppNavigation/GroupNavigationItem.vue 0.00% <ø> (ø)
src/components/ContactsList.vue 0.00% <ø> (ø)
src/store/contacts.js 5.10% <0.00%> (ø)
src/models/contact.js 12.79% <100.00%> (ø)
src/models/rfcProps.js 62.50% <100.00%> (ø)
src/services/updateDesignSet.js 69.23% <100.00%> (ø)
...mponents/ContactDetails/ContactDetailsProperty.vue 0.00% <0.00%> (ø)
src/services/isContactsInteractionEnabled.js 0.00% <0.00%> (ø)
src/store/index.js 80.00% <0.00%> (ø)
src/services/collaborationAutocompletion.js 0.00% <0.00%> (ø)
... and 72 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67a9da3...b34df2d. Read the comment docs.

@miaulalala
Copy link
Contributor

This doesn't solve cases where the type string looks like this: TYPE="HOME","VOICE" but I don't think that this would be RFC compliant in the first place.

@miaulalala miaulalala marked this pull request as ready for review February 10, 2022 10:35
@miaulalala miaulalala requested a review from st3iny February 10, 2022 10:35
@miaulalala miaulalala self-assigned this Feb 15, 2022
@miaulalala miaulalala added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 15, 2022
@skjnldsv skjnldsv requested review from miaulalala and hanzi February 21, 2022 09:54
@skjnldsv
Copy link
Member Author

Maybe some testing? 🙈

@miaulalala
Copy link
Contributor

Maybe some testing? see_no_evil

tell me how? 😬

@skjnldsv
Copy link
Member Author

Oh wow, we have zero js testing 🙈
If you feel like adding a very basic one to test the src/models/contact.js file

@miaulalala miaulalala added this to the v4.0.9 milestone Feb 28, 2022
@rexbron
Copy link
Contributor

rexbron commented Mar 10, 2022

This is a big quality of life improvement for iOS users, anything that can be done to move it along?

@skjnldsv
Copy link
Member Author

/rebase

@miaulalala
Copy link
Contributor

@skjnldsv I have no ida how to add a testing framework to the app at all. This needs to be done by somebody who has JS dev experience - so, not me.

@skjnldsv skjnldsv force-pushed the fix/type-quting branch 3 times, most recently from 9669770 to 756a9e6 Compare March 15, 2022 15:58
Signed-off-by: John Molakvoæ <[email protected]>
Signed-off-by: John Molakvoæ <[email protected]>
@miaulalala miaulalala modified the milestones: v4.0.9, v4.1.0 Mar 15, 2022
@skjnldsv skjnldsv merged commit 6d171ad into main Mar 15, 2022
@skjnldsv skjnldsv deleted the fix/type-quting branch March 15, 2022 18:04
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 15, 2022
@moeb98
Copy link

moeb98 commented May 23, 2022

Sure, this is fixed? Installed NC 23.0.5 and Contacts 4.1.1... still effect that categories on iOS 15.5 are shown as "HOME or "CELL, etc...

@rexbron
Copy link
Contributor

rexbron commented May 23, 2022

Sure, this is fixed? Installed NC 23.0.5 and Contacts 4.1.1... still effect that categories on iOS 15.5 are shown as "HOME or "CELL, etc...

What I've noticed is that iOS and the CardDav server will sometimes exchange the old incorrectly quoted label as a custom label. What has worked for me is reapply the mobile lable in the web interface and then immediately trigger a sync on contacts. Very annoying that this bug polluted the database but alas.

@moeb98
Copy link

moeb98 commented May 23, 2022

Sure, this is fixed? Installed NC 23.0.5 and Contacts 4.1.1... still effect that categories on iOS 15.5 are shown as "HOME or "CELL, etc...

What I've noticed is that iOS and the CardDav server will sometimes exchange the old incorrectly quoted label as a custom label. What has worked for me is reapply the mobile lable in the web interface and then immediately trigger a sync on contacts. Very annoying that this bug polluted the database but alas.

Hmmm... didn't work on my end (assume you meant the NC web interface... not iCloud, right?). At the same time, this would imply to go through hundreds / thousands of contact labels (depending on your address book size).

If this is due to a polluted database, isn't there an occ command (as a workaround at least)?

@moeb98
Copy link

moeb98 commented Jun 13, 2022

Any ideas / hints?

@OmarWazzan
Copy link

@moeb98 Were you able to figure anything out for this?

@moeb98
Copy link

moeb98 commented Jul 20, 2022

Nope, after waiting for any feedback here for a few days, I honestly lost track due to other priorities and rather switched back to the old iCloud sync using NC for calendar only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug Something isn't working technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove quotes from TYPE for better external platforms compatibility
6 participants