Don't allow to save empty URI

[4dfe-9dac]

The title says it all, don't let people save empty URI.

If empty give them an error before saving it or remove the URI

[dmcfaddengalway]

I'd love to work on this one.

[eliykat]

Go for it @dmcfaddengalway! Please @ me in the PR.

[dmcfaddengalway]

@eliykat Thanks!

[f09f909b]

Hello!

It has been a while since an update in this thread. Would this be okay for me to work on?

[enmanuelmag]

Hi, I could work on it @k4awon

[ghost]

Here is some code to speed up the process.

function main(): void {
    // Check if the string is empty
    console.log(isStringEmpty("Hello World"))
}

main()

// Check if a given string is empty and return a boolean
function isStringEmpty(content: string): boolean {
    return content.length == 0
}

Just do a basic check and if its empty than don't save it.

[f09f909b]

Hey @enmanuelmag I have been busy! You can go for it!

[eliykat]

FYI, we have Utils.IsNullOrWhitespace which might be useful here, to save you defining your own method.

[Matheus-BM]

Hi, I've notice the issue remain open. Can i help?

[eliykat]

Hi @Matheus-BM, there hasn't been much activity here so I'd say go for it. However, we're in the middle of reorganizing our repositories (more info here), so I'd recommend waiting a week or so before forking any code. Keep an eye on that Community Forums thread for updates in the meantime.

[jbeachy21]

Hi newcomer here, is this possibly up for grabs?

[Matheus-BM]

Hi newcomer here, is this possibly up for grabs?

Hi, I've almost solved the issue, but notice that the project didn't have any error messages attached to the translations that I could show when trying to save empty URI, I was going to request to a new error message, but I were waiting for the migration to the mono repository to end and lost track of time, if you wanna help me open the request for a new string on crowding it would be nice.

[eliykat]

If you need a new translation string, you can just add it to src/app/locales/en/messages.json, using the format in that file. That's the "base" translation file. It will be automatically synced with CloudIn, and translators will update all other messages.json files.

[Matheus-BM]

Nice, btw @eliykat one question, if i want to add the solution among other verification like nameRequired in browser\jslib\angular\src\components\add-edit.component.ts which is in jslib repository how should i proceed with the pull request?

[eliykat]

@Matheus-BM contributions are definitely welcome, however with the repo changes still underway, I think it's probably best to hold off. We intend to migrate web, cli and jslib into this repo (bitwarden/clients) by about 1 June. Then you'll be able to submit a single PR that updates jslib and any client/s at the same time (part of the reason why we're making this change!)

Feel free to still experiment and test things locally during this time - just keep an eye out for when we've finished our migration before submitting a PR.

[njmulsqb]

Hi, is anyone working on this issue? I am willing to work on it but having build issues, I see just recently desktop and browser repos have been merged. How to build this project now? Given that there are multiple projects in this repo, I have ran npm i in desktop directory as well but it failed too

If this issue is currently being worked on, point me towards some other one but build errors needs to be tackled first. I am using npm i in the root of this repo and its failing

[eliykat]

Hi @njmulsqb, I responded to your thread in the Community Forums, but for anyone else also struggling with build steps: check out the build instructions at https://contributing.bitwarden.com/clients/.

Best to wait and see whether @Matheus-BM still wants to work on it. The monorepo transition is complete, so that's no longer an obstacle.

[Matheus-BM]

Hi, @eliykat and @njmulsqb I still want to work on the issue, was just waiting the merge to finish. I probably will submit the pull request later this weekend.

[Matheus-BM]

@njmulsqb if you still looking for a issue to work on, you can have this one, I've solved the issue before mono-repo were done, but now I'm having problems with npm ci , checked instructions but no clue whats happening. Anyway, I no longer want to delay the solving of this issue, if you need the changes I've made I can send it to you.

[njmulsqb]

Hi @Matheus-BM , Yes sure you can send me the changes you have made and I am ready to work on it. Lately, I am also having build errors when I run npm run electron, here's what the output looks like: https://pastebin.com/eY1tKHn7 Does this seem relevant to the problem you're facing?

[njmulsqb]

@Matheus-BM Can you share the changes you made? @eliykat Can you have a look at the pastebin I shared above? I am unable to build desktop app lately, I have taken fresh pull from this repo, npm ci in the root of repo and then npm run electron in desktop directory and its failing. Is it a known issue? How are other devs tackling with this?

[Matheus-BM]

I've only replaced an old if with this code at line 280 from add-edit.components.ts at libs\angular\src\components\add-edit.component.ts

   for(let i =0; i< this.cipher.login.uris.length ; i++){
   
      if (
        this.cipher.type === CipherType.Login &&
        this.cipher.login.uris != null &&
        (Utils.isNullOrWhitespace(this.cipher.login.uris[i].uri) ) 
      ) {
        this.platformUtilsService.showToast(
          "error",
          this.i18nService.t("errorOccurred"),
          this.i18nService.t("uriEmpty")
        );
        return false;
      }
    }

and added the new error message "uriEmpty" at apps\browser\src\_locales\en\messages.json

 "uriEmpty": {
"message": "URI cannot be empty."}

[Matheus-BM]

As for the electron error on desktop build, not sure its an tracked issue or how to fix it. I've been working on the web extension, but now can't even complete the npm ci because some compatibility issue.

[njmulsqb]

I can do npm ci but fails at npm run electron. Lets wait for some dev to respond

[eliykat]

@njmulsqb I'm not sure what's causing that error, unfortunately the output is not very descriptive. Are you on MacOS or Linux?

Some suggestions:

• Are you using the correct version of node? i.e. node v16 and npm v8
• Have you installed all the Requirements, depending on your OS?

[njmulsqb]

@eliykat I am on Linux WSL.

Node version is 16.14.2 NPM version is 8.12.2

[djsmith85]

@eliykat I am on Linux WSL.

Node version is 16.14.2 NPM version is 8.12.2

@njmulsqb As you are trying to run npm run electron I'm assuming you are trying to launch the desktop client. A long time ago, I also used WSL2 for my dev environment. The thing with WSL is, that it at time at least, did not come with a window manager. Which is needed to draw/display a window. To overcome this you will need an Xserver. I think I went with VcsXsrv. It did take me a while to get everything setup and it was not very fast so now I do things directly on Win. One less thing to worry about.

@Matheus-BM Not sure what kind of issues you are running into with installing npm packages.

I suggest possibly discussing this further over at https://community.bitwarden.com/t/help-needed-regarding-contributing-to-bitwarden-clients/42004 so we don't spam this issue with setup questions.

[adradan]

I'd love to pickup this issue if it is no longer being worked on by anyone else.

[nick-cd]

Hello! I'm just curious about the implementation preference for this issue.

The author in the original post suggests two possible solutions: prune empty URIs or issue an error message upon saving. Out of curiosity, which of these two would be your preferred approach, @eliykat?

I see attempts from other collaborators have opted for the latter solution, so you're likely content with that. Though, I'm still interested in hearing a definitive answer on which solution you think is more appropriate.

[eliykat]

I personally prefer silently pruning empty URIs upon save. There's no use case for saving an empty URI, so no need to bother the user with an error message in my opinion. Happy to discuss if you see any problem with that approach though.