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

Serialize and deserialize ORGANIZER with params #47

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ryanwinchester
Copy link
Contributor

@ryanwinchester ryanwinchester commented Nov 15, 2020

Closes #46

This serializes and desializes ORGANIZER similar to how ATTENDEE works.

The only problem is that this is a breaking change for deserializing since it's now a map (like attendee) and not a string.

One way we could make deserializing not a breaking change would be to pass in some "optional options" opts \\ [], but they'd have to be passed all the way through from:
ICalendar.Deserialize.from_ics/1
-> ICalendar.Util.Deserialize.build_event/1
-> ICalendar.Util.Deserialize.parse_attrs/2

What do you think we should do?

@@ -74,6 +74,7 @@ defmodule ICalendar.Util.DeserializeTest do
assert %Event{} = event
end

@tag :skip
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this skipped?

Copy link
Contributor Author

@ryanwinchester ryanwinchester Nov 21, 2020

Choose a reason for hiding this comment

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

This is to show that it is a breaking change. It no longer deserializes to a string. It deserializes to a map. This is the original test, I added a new test just below this one for the new functionality.

assert event.organizer == "mailto:[email protected]"

Will fail, now.

I left this in (but skipped so we have passing tests) so you can compare the old functionality with the new.

Copy link
Owner

Choose a reason for hiding this comment

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

Let's update this test to pass now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's update this test to pass now

I duplicated in the test below with the new functionality.

test "Include ORGANIZER w/ params and ATTENDEEs in event" do
event =
"""
BEGIN:VEVENT
DTSTART:20190711T130000Z
DTEND:20190711T150000Z
DTSTAMP:20190719T195201Z
ORGANIZER;[email protected]:mailto:[email protected]
UID:7E212264-C604-4071-892B-E0A28048F1BA
ATTENDEE;CUTYPE=INDIVIDUAL;ROLE=REQ-PARTICIPANT;PARTSTAT=ACCEPTED;[email protected];X-NUM-GUESTS=0:mailto:[email protected]
ATTENDEE;CUTYPE=INDIVIDUAL;ROLE=REQ-PARTICIPANT;PARTSTAT=ACCEPTED;[email protected];X-NUM-GUESTS=0:mailto:[email protected]
ATTENDEE;CUTYPE=INDIVIDUAL;ROLE=REQ-PARTICIPANT;PARTSTAT=ACCEPTED;CN=James SM;X-NUM-GUESTS=0:mailto:[email protected]
CREATED:20190709T192336Z
DESCRIPTION:
LAST-MODIFIED:20190711T130843Z
LOCATION:In-person at Volta and https://zoom.us/j/12345678
SEQUENCE:0
STATUS:CONFIRMED
SUMMARY:Design session
END:VEVENT
"""
|> String.trim()
|> String.split("\n")
|> Deserialize.build_event()
assert %Event{organizer: organizer} = event
assert %{:original_value => "mailto:[email protected]", "CN" => "[email protected]"} = organizer
end

So we can just delete this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpil removed

lib/icalendar/util/deserialize.ex Show resolved Hide resolved
@jarrodmoldrich
Copy link

jarrodmoldrich commented Oct 21, 2021

Hi @lpil and @ryanwinchester 👋 I see this PR has stagnated, and I'm realising it's something I need. I think the delta is pretty small for a feature change like this, so I think with a small tweak we can maintain backwards compatibility and push this through.

One way we could make deserializing not a breaking change would be to pass in some "optional options"

The small tweak I'd suggest is simply using Elixir style function pattern matching

def build("ORGANIZER", %{} = organizer) do
    params =
      for {key, val} <- organizer, key != :original_value, into: "" do
        ";#{key}=#{sanitize(val)}"
      end

    "ORGANIZER#{params}:#{organizer.original_value}\n"
end 

def build("ORGANIZER" = key, value) when is_binary(value) do
   "#{key}:#{Value.to_ics(value)}\n"  
end

So just copying the vanilla def build(key, value) fallback implementation. We could even extract out the implementation out into an internal function like defp build_raw(key, value) and call it from both. This way it won't break backwards compatibility. I also added a sanitize for the parameter values, as injected commas and semi-colons could break the file.

Does all that make sense? Or have I misjudged? I'm happy to make these changes and update the PR. At the second I'm lacking the time, but I could contribute to maintenance, if you're wanting to share some of the responsibility @lpil

In the meantime I'll make these changes on my own fork. Let me know.

@ryanwinchester
Copy link
Contributor Author

@jarrodmoldrich if you add this test back in, with your changes, does it pass?

79b9159#diff-38283140e12894618fc3e09cccbe6d5aa4bfc35942eb9da277360ede3077e77dL78-L105

@jarrodmoldrich
Copy link

I just ran mix test on my branch from main, which includes the old test (but not your new one) and it passed. The code is here:

jarrodmoldrich@4cbeaff

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.

Serializing with Organizer should have a special case in KV, like Attendees
3 participants